-
Notifications
You must be signed in to change notification settings - Fork 842
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
EuiBottomBar: allow customization of whether the component makes room for itself #3641
EuiBottomBar: allow customization of whether the component makes room for itself #3641
Conversation
* Add new `affordForDisplacement` prop (default: true) to allow customization of whether the component makes room for itself by adding padding equivalent to its own height on the document body element. Use case a web app which ordinarily doesn't require vertical scrollbars (with a large enough browser viewport) - the existing unconfigurable behavior was forcing a vertical scrollbar to appear in this scenario, as can be seen in the updated component doc page. * Make clear that the `paddingSize` prop default is "m".
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? |
jenkins test this |
Chromium failed. Jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3641/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this one. Let's not add this to EuiControlBar just yet. We've been implementing some similar behaviors with fixed EuiHeader's and would rather rely on SASS mixins when we can.
* Whether the component should apply padding on the document body element to afford for its own displacement height. | ||
* Default is true. | ||
*/ | ||
affordForDisplacement: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this prop?
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Hey @dannya , Just wondering if you were still going to wrap up this PR or if you need help getting it over the line? I think all it really needs are some tests. |
Hi @cchaos , I did start on the tests for this, but wasn't sure of the best approach for testing this specific new functionality (because it would need to test an effect external to the component, in this case the My understanding is that typical Jest tests and snapshots are only testing the rendered component HTML, rather than in the context of a full HTML page (with Any ideas on a way of testing this specific case? Thanks, |
Yes, that is ok for now (testing simply the component render). Mainly we want to ensure that the component doesn't error out when using the prop appropriately. We can always circle back to testing of the actual behavior, but for now the simple snapshot is fine. |
Hi @dannya I was finishing up this PR for you and attempted to push my changes directly but permissions were denied so I've created a new PR based off this one and going to close this one directly. |
affordForDisplacement
prop (default: true) to allow customization of whether the component makes room for itself by adding padding equivalent to its own height on the document body element.Use case is a web app which ordinarily doesn't require vertical scrollbars (with a large enough browser viewport) - the existing unconfigurable behavior was forcing a vertical scrollbar to appear in this scenario, as can be seen in the updated component doc page.
paddingSize
prop default is "m".Summary
The most relevant previous code change this builds off is #680 by @snide
I understand that there is the facility to specify a custom
className
to effect this, but my rationale here is two-fold - it feels inelegant to have to set a custom CSS class with an!important
rule in order to override the existing behavior (as it is achieved with inline styles), and IMO it is worthwhile having an explicit ability to opt-out of this behavior (whilst keeping the existing behavior as the default).I've seen (quite late into making this change) that there is a similar EuiControlBar component which seems to be favored for future development effort - the same mechanism for automatically adding a height displacement occurs there too. If you are satisfied with my changes here, I will make the same component prop addition in that component, and then update the
CHANGELOG
.Checklist