Use the whole list item as the click target#3963
Conversation
| <li | ||
| className={classNames('filterNavigatorBarItem', { | ||
| filterNavigatorBarRootItem: i === 0, | ||
| filterNavigatorBarBeforeSelectedItem: i === selectedItem - 1, |
There was a problem hiding this comment.
I removed this class as it was clearly not used.
Codecov Report
@@ Coverage Diff @@
## main #3963 +/- ##
=======================================
Coverage 87.26% 87.26%
=======================================
Files 279 279
Lines 23921 23924 +3
Branches 6315 6317 +2
=======================================
+ Hits 20874 20877 +3
Misses 2807 2807
Partials 240 240
Continue to review full report at Codecov.
|
| className={classNames('filterNavigatorBarItem', { | ||
| filterNavigatorBarRootItem: i === 0, | ||
| filterNavigatorBarBeforeSelectedItem: i === selectedItem - 1, | ||
| filterNavigatorBarSelectedItem: i === selectedItem, |
There was a problem hiding this comment.
I considered removing this idea of selectedItem as currently it's always the last item. But finally I decided to keep it just in case we were using it in the future.
| <li | ||
| className={classNames('filterNavigatorBarItem', { | ||
| filterNavigatorBarRootItem: i === 0, | ||
| filterNavigatorBarBeforeSelectedItem: i === selectedItem - 1, |
| 'filterNavigatorBarItem', | ||
| 'filterNavigatorBarLeafItem', | ||
| 'filterNavigatorBarUncommittedItem' | ||
| )} |
There was a problem hiding this comment.
Good idea to move that inside the FilterNavigatorBarListItem component!
| isLastItem={true} | ||
| isSelectedItem={false} | ||
| additionalClassName="filterNavigatorBarUncommittedItem" | ||
| title={uncommittedItem} |
There was a problem hiding this comment.
Huh, interesting that we are putting a title only for uncommited items, and this is exactly the same text we put inside the button as well 🤔 I feel like this is redundant (but not a blocker for this PR since this was a preexisting issue).
There was a problem hiding this comment.
Yeah I wondered about that too...
I believe we don't do it for other items directly currently because they're <Localized> React nodes, so they're not pure strings that we can reuse. I believe we had titles for everything in the past, and it could be useful in the transform navigator (not so much for the range navigator). Indeed in the transform navigator the text can be cut with an ellipsis sometimes.
If we want to do them in a localizable way, then this would need some more work, I believe that's why it hasn't been done earlier.
2 participants
This was caused by my previous patch #3901. Indeed I moved the
onClickhandler from the<li>to the<button>. This patch reverts this changes but still keeps the behavior that #3901 fixed.Fixes #3962
production
deploy preview
Things to try: