♻️ (tooltip) refactoring to react-aria (vol.3) #2631
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
MatissJanis
changed the title
trafico-bot
bot
added
the
🚧 WIP
MatissJanis
changed the title
trafico-bot
bot
added
🔍 Ready for Review
|
Out for review now.
|
|
Would it be possible to retain the old scrollbars? Old (Demo link): New (This PR): Observations:
|
|
Scrollbar: I pushed a change. Does the scrollbars work as expected now? Totally agree with you that we need to get this patched before merging this PR. But sadly I'm not able to reproduce on OSX, so fixing it is very challenging.
This one's tricky and I think we need to make a decision here on whether we proceed with the migration to Here's my investigation: When we use stacked popovers (filters popover + autocomplete) - only the topmost popover can be closed when clicking on the outside. This is also assuming none of the popovers use the However, we do want to use
So to sum it up: we have to decide if we wish to proceed with the migration given the functional change that you discovered. Clicking outside the filters menu while the autocomplete is open does not close the filter menu. WDYT? |
|
The scroll bars are all good now!
I'm good with this change in behavior. I just noticed something weird when the filter auto complete is open. When I click outside the filter popover, the auto complete popover won't close - it just returns focus to the auto complete input field. Only when click on the sidebar then that's when it closes. |
|
Im still getting the basic scroll bars in Brave on Linux. Firefox is working though |
Could you help me out here and submit a commit for the fix here? Normally I'd be happy to work on it, but I have only OSX :/ |
|
Are the scrollbar's appearance supposed to be unchanged from the current demo? And on Chrome (Windows 10): |
They are supposed to not change, but are currently stuck trying to figure out what the problem is |
|
Ive been poking around this scrollbar issue. I get this error in the console. I tried changing the property name like it suggests, but nothing changed other than the error went away. Error |
|
Pushed a small update for the scrollbars. Let me know how it looks like now on your devices. 🤞 |
|
Its definitely better, but still not the same as before |
|
Still the same. Interestingly, it works when in responsive mode, even when viewing the desktop style view. |
|
The scrollbar on the budget page is broken now. I don't know when that started, or if its happening to anyone else. It doesn't happen in the demo, so its probably from this PR. As is the pattern, it looks fine in firefox. It's different than before, thinner than previous, but still looks fine. |
|
Pushed an update. The scrollbar changes will now apply only to the new popovers. Which should make budget page work just as it did before. Though I still don't have ideas how to reduce the scrollbar width to match the one we had before. |
|
Ok, that fixed the budget page scrollbars |
| style={{ | ||
| padding: 0, | ||
| ...styles.darkScrollbar, |
There was a problem hiding this comment.
Instead of putting this in a style prop, can we try using glamor to pass this to className? Maybe the inline style prop doesn't support pseudo selectors. Another possible option is to just use the scrollbarWidth and scrollColor css props instead of the darkScrollbar.
There was a problem hiding this comment.
Using glamor is a good idea! Updated it now, let me know how it looks!
There was a problem hiding this comment.
It still is not working, we need to check that the darkScrollbar is set in the children's container element.
| style={{ | ||
| padding: 0, | ||
| ...styles.darkScrollbar, |
There was a problem hiding this comment.
It still is not working, we need to check that the darkScrollbar is set in the children's container element.
| }} | ||
| {...tooltipProps} | ||
| data-testid="autocomplete" | ||
| > | ||
| {renderItems( |
There was a problem hiding this comment.
Maybe we can try wrapping this in a View and setting the scrollbar style there?
There was a problem hiding this comment.
Would you be open to try this out locally? I'm working blindly here without a way to reproduce.
| style={{ ...styles.tooltip, padding: 0, ...style }} | ||
| className={`${css({ | ||
| ...styles.tooltip, | ||
| ...styles.lightScrollbar, |
There was a problem hiding this comment.
I think the old tooltip component defaults to using darkScrollbar
There was a problem hiding this comment.
Yea, it did. But I think that was largely an unnoticed minor bug.
There are only 2x places where tooltips have scrollable content: autocomplete (where it makes sense to have darkScrollbar as the autocomplete is dark itself) and notes (where it makes more sense to have it light IMO).
|
What's the verdict here from you? Shall we proceed with the refactors to react-aria (which has some scrollbar changes) or would you prefer to roll-back the refactors already done (mostly just notes popovers)? AFAIK the only blocker at this point is the scrollbar width change for MS/Linux devices (colors should be correct now?). If we decide to proceed with the migration - I'd create a separate issue to address the scrollbar widths and I'd hope on of yous could pick it up (as again: I do not have a way to reproduce the issue on Mac). Let me know your thoughts. |
|
@MatissJanis @youngcw Pushed an update for the scrollbar. Can you check? The bulk edit modal styling is off: |
|
The scroll bars are fixed for me now. I am also seeing the overlap in the bulk edit modal |
|
Thanks @joel-jeremy for figuring out the solution there! Bulk edit modal should now be fixed. |
|
Looks good to me |
trafico-bot
bot
added
✅ Approved
|
Thanks for all the help folks! |
trafico-bot
bot
added
✨ Merged
Follow up to #2493
This refactors the autocompletes, date selects and the "filters" menu.