Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grouped Trace tabs #312

Merged
merged 8 commits into from
Feb 8, 2018
Merged

Grouped Trace tabs #312

merged 8 commits into from
Feb 8, 2018

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.

@@ -106,7 +108,7 @@ class Panel extends Component {
);

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