-
Notifications
You must be signed in to change notification settings - Fork 974
Fix bookmarks toolbar overflow indicator display #9664
Fix bookmarks toolbar overflow indicator display #9664
Conversation
@bsclifton Could you take a look? |
@GreenRecycleBin could you pls rebase? |
06ecb04
to
398a3cc
Compare
@cezaraugusto A recent commit fixed the regression introduced by an earlier commit. I've removed one of my previous commit that did the same thing and rebased the other onto master. |
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.
you're in the right track btw but calculation still needs some tweaks. let me know if you need a hand |
Hey there @GreenRecycleBin- it's been a while, I just wanted to follow up with you on this one @cezaraugusto had left some feedback (above). Have you gotten a chance to look at it again? Please let us know if you need a hand 😄 |
I noticed The first two obviously need to be in sync according to this comment and the other one. Should we keep these bookmark related styles in sync across these three files? If so, what's the best way to do that? Thank you. |
@GreenRecycleBin we're in the process of moving things out of LESS and make use of Aphrodite. Styles shared across components should live in All LESS files and You can find a good example in bookmarkToolbarButton. You can move let me know if that makes sense for you |
398a3cc
to
e6d5d02
Compare
@cezaraugusto Could you take another look? |
e6d5d02
to
273f01f
Compare
273f01f
to
f291eb3
Compare
@cezaraugusto There were some conflicts with |
app/common/lib/bookmarkUtil.js
Outdated
// Toolbar padding is only on the left | ||
const toolbarPadding = parseInt(globalStyles.spacing.bookmarksToolbarPadding) | ||
|
||
const overflowButtonWidth = 25 |
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.
I know it was like this before but could you please add this to globalStyles
as well?
const bookmarkItemMaxWidth = '100px' | ||
const bookmarkItemPadding = '4px' | ||
const bookmarkItemMargin = '3px' | ||
const bookmarkItemChevronMargin = '4px' | ||
const bookmarkToolbarButtonDraggingMargin = '25px' |
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.
this could benefit from the above comment as well
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.
I did a stress test for the 3 cases we have and coudn't replicate the issue again, awesome work! I left a nit comment that I'd like to be done, but otherwise this looks great.
@cezaraugusto Could you take another look? If everything works, could you do |
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.
++
@GreenRecycleBin thanks for hanging in there and delivering the fix! 😄 |
Fix #6869
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests