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

Allowed variable gutterSize in EuiFacetGroup #3639

Merged
merged 5 commits into from
Jun 24, 2020
Merged

Conversation

shrey
Copy link
Contributor

@shrey shrey commented Jun 19, 2020

Summary

Allowing user to choose the size of gutter in EuiFacetGroup, by adding gutterSize
Closes #3621

Checklist

@snide We can put different margin for horizontal and vertical, although that might not be of much use and make the sass code redundant, if you like these changes, I'll add the tests :)

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@shrey shrey changed the title Allowed variable padding in EuiFacetGroup Allowed variable gutterSize in EuiFacetGroup Jun 19, 2020
@miukimiu
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3639/

@shrey
Copy link
Contributor Author

shrey commented Jun 23, 2020

@miukimiu I updated and wrote the tests, any idea why the tests failed?

@miukimiu
Copy link
Contributor

jenkins test this

@miukimiu
Copy link
Contributor

@miukimiu I updated and wrote the tests, any idea why the tests failed?

The tests failed for this reason: ERROR: Failed to download Chromium r706915! Set "PUPPETEER_SKIP_CHROMIUM_DOWNLOAD" env variable to skip download. It happens sometimes.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3639/

@shrey
Copy link
Contributor Author

shrey commented Jun 23, 2020

@miukimiu so, Do I need to do something, or you'll merge it?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

@shrey Thanks for getting this one started, though it looks like this one is going to take some finessing. Just adding margins to the left and bottom of the current display adds way too much white space between items stemming from the height of the button itself as you had mentioned in the issue.

Before

Screen Shot 2020-06-23 at 10 15 56 AM

After

Screen Shot 2020-06-23 at 10 16 04 AM


Also, vertical aligned items shouldn't have any margin to the right. And horizontal layout should account for collapsing any bottom margin.

Just give us a shout if you need help getting this one over the line.

src/components/facet/_facet_group.scss Outdated Show resolved Hide resolved
src/components/facet/_facet_group.scss Outdated Show resolved Hide resolved
src/components/facet/facet_group.test.tsx Outdated Show resolved Hide resolved
src/components/facet/facet_group.tsx Outdated Show resolved Hide resolved
src/components/facet/facet_group.tsx Outdated Show resolved Hide resolved
@shrey
Copy link
Contributor Author

shrey commented Jun 23, 2020

@shrey Thanks for getting this one started, though it looks like this one is going to take some finessing. Just adding margins to the left and bottom of the current display adds way too much white space between items stemming from the height of the button itself as you had mentioned in the issue.

Before

Screen Shot 2020-06-23 at 10 15 56 AM

After

Screen Shot 2020-06-23 at 10 16 04 AM

Also, vertical aligned items shouldn't have any margin to the right. And horizontal layout should account for collapsing any bottom margin.

Just give us a shout if you need help getting this one over the line.

if not margin, how should I add spacing?

@cchaos
Copy link
Contributor

cchaos commented Jun 23, 2020

It has to be margin but you'll need to adjust the overall height of the buttons. This one is a bit design-y so again, let us know if you want one of us to jump in.

@shrey
Copy link
Contributor Author

shrey commented Jun 23, 2020

@cchaos It'd be great if you guys could jump in :)

@cchaos
Copy link
Contributor

cchaos commented Jun 23, 2020

I've pushed a commit that takes a design pass at the different gutter sizing and adjusts some examples.
Jenkins, test this

@cchaos cchaos requested a review from miukimiu June 23, 2020 19:37
@cchaos
Copy link
Contributor

cchaos commented Jun 23, 2020

retest

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3639/

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

I looked at the code and tested locally. LGMT! 🎉

shrey and others added 5 commits June 24, 2020 15:34
- Reducing gutter sizes to just none, small, medium, and large
- Adjusting that spacing appropriately
- Fleshed out tests
- Added `innerText` to facet buttons in case of truncation
- Fixed color example
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM 2 :) Will merge on green

@cchaos
Copy link
Contributor

cchaos commented Jun 24, 2020

retest

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3639/

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.

Add gutterSize prop to EuiFacetGroup
4 participants