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

Remove EuiFlexGroup dependency from EuiAccordion #2143

Merged
merged 6 commits into from
Jul 22, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 19, 2019

Based on the issues stemming from negative margins and all the different options from EuiFlexGroup and wild nesting, I've removed this dependency for just applying display: flex in CSS.

Ref: #2141

Fixes the failed responsiveness when using the extraAction prop

Screen Shot 2019-07-19 at 11 23 02 AM

I also added an example of how to truncate the text properly

Screen Shot 2019-07-19 at 11 23 16 AM

cc @sulemanof @maryia-lapata

Checklist

  • This was checked in Kibana
  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • [ ] This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • [ ] This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

@cchaos cchaos requested review from snide, thompsongl and sulemanof July 19, 2019 16:06
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

I think approach is good and looks fine on the code side. Anytime we change flex though I think it's smart to test further downstream. Let's make sure we run this in Kibana and checking some known usages before merging.

@cchaos
Copy link
Contributor Author

cchaos commented Jul 19, 2019

I checked this in current instances of EuiAccordion in Kibana as well as @maryia-lapata's PR. All the current instances are good, and so is the PR with some tweaking of the button content.

@cchaos cchaos requested a review from snide July 19, 2019 21:21
@cchaos cchaos force-pushed the accordion-remove-flexgroup branch from 4f663e8 to 92282d5 Compare July 20, 2019 04:22
className={buttonClasses}
type="button">
<span className="euiAccordion__iconWrapper">{icon}</span>
<span className={buttonContentClasses}>{buttonContent}</span>
Copy link
Contributor

@sulemanof sulemanof Jul 22, 2019

Choose a reason for hiding this comment

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

The buttonContentClasses contains the buttonContentClassName prop and euiAccordion__buttonContent class, which I can't find in the existing styles. Could we get rid of buttonContentClassName at all since we can pass a buttonContent node with already applied class ? And also get rid of redundant buttonContentClasses variable ..

Suggested change
<span className={buttonContentClasses}>{buttonContent}</span>
{buttonContent}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point about the class .euiAccordion__buttonContent not being used in any SASS, I can remove that class. However, we need to keep buttonContentClassName because as a consumer, you can't target that inner span wihout overriding a .eui class.

So we're providing a way to target that exact span, which needs to happen if you need to truncate the text, as is shown in this example:

buttonContentClassName="eui-textTruncate"

Screen Shot 2019-07-22 at 09 38 56 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

What is a benefit of providing a className for a node which I also provide manually? That means I could pass the buttonContent prop as node : <span className="eui-textTruncate">My very very very long text is here</span>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because not all consumers know to do it that way, haha

@cchaos
Copy link
Contributor Author

cchaos commented Jul 22, 2019

retest

@cchaos
Copy link
Contributor Author

cchaos commented Jul 22, 2019

jenkins, test this

@cchaos cchaos merged commit 4f4a698 into elastic:master Jul 22, 2019
@cchaos cchaos deleted the accordion-remove-flexgroup branch July 22, 2019 15:34
@snide snide mentioned this pull request Jul 22, 2019
7 tasks
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.

4 participants