Skip to content

Grouped Trace tabs#312

Merged
nicolaskruchten merged 8 commits intomasterfrom
trace_tabs
Feb 8, 2018
Merged

Grouped Trace tabs#312
nicolaskruchten merged 8 commits intomasterfrom
trace_tabs

Conversation

@nicolaskruchten
Copy link
Contributor

This PR replaces #160. Multi-value UX similar to Axes panel yet to come, in another PR :)

@nicolaskruchten
Copy link
Contributor Author

Oh I should mention that trace type names in a group context are not localized (or even properly capitalized!) yet but that will come once #311 is merged and I'll be able to leverage those localized constants from somewhere.

@nicolaskruchten
Copy link
Contributor Author

I've added an item to #290 to capture this.


return (
<div className={bem('panel')}>
<div className={`panel${tabs ? ' panel--with-tabs' : ''}`}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think we're not really using that bem stuff, we should probably remove it at some point..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added by @aulneau so we should get his input... we should either do one big pass where we remove it or add it everywhere in a single PR but agreed that we should standardize!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no : ) this was added before aulneau, we've just all been adding to it, but yeah @aulneau what do you think about that bem utility, useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, did you mean the bem() function or the actual BEM pattern?

And yes, the particular edit you're commenting on came from Thomas ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually come to think of it I was going to refactor it a bit, good that you chose this one to comment on!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the same family of functions as things like bem() I like https://github.com/JedWatson/classnames FWIW...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the bem function, which was introduced before @aulneau started contributing to this project : )

@VeraZab
Copy link
Contributor

VeraZab commented Feb 8, 2018

This is clean! I'm just going to check out this branch and test it out, but 💃

@nicolaskruchten nicolaskruchten merged commit a061c56 into master Feb 8, 2018
@nicolaskruchten nicolaskruchten deleted the trace_tabs branch February 8, 2018 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants