-
Notifications
You must be signed in to change notification settings - Fork 267
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
Add margin to prevent "Clear all btn" from moving fix #2062 #2217
Conversation
Hi @yamaken1343. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 the PR @yamaken1343. It looks like this change pushes the button too far down when there is a tag displayed to the left of it.
Instead of the 16px margin top which we add to the tags, it looks like the button only needs about 11px (due to different height of the button vs. tag). If there's a way to achieve this without hardcoding the 11px value that would be preferred, but if not we can hardcode it for now.
/ok-to-test
I add margin to `.bx--btn` class to prevent "Clear all btn" from moving. The "filter" has `.bx--tag` class and the "Clear All button" has `.bx--btn` class. `.bx--tag` class has margin but `.bx--btn` class has not margin. So, there was a gap between when there was a "filter" on the left and when there was no "filter". fix tektoncd#2062
Tag height is 1.5rem and tag margin is 1rem. Btn height is 2rem. So btn margin should be 0.75rem correct. But 0.75rem is 12px, not 11px. |
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 @yamaken1343, this looks good.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlanGreene The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thank you for review! |
Changes
fix #2062
The "filter" has
.bx--tag
class and the "Clear All button" has.bx--btn
class..bx--tag
class has margin but.bx--btn
class has not margin. So, there was a gap between when there was a "filter" on the left and when there was no "filter".I add margin to
.bx--btn
class to prevent "Clear all button" from moving.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.