fix: repair the image gallery sort control - #3219
Conversation
The gallery sort control reads "Bynewest" instead of "By newest". The space lived in a literal JSX text node, and dprint stripped it in openedx#2997 when it replaced eslint. An expression container survives formatting. The existing tests match on the accessible name, which the accname algorithm space-joins across elements, so they never saw the missing space.
SelectMenu hard-codes a bottom-start placement and its menu is 19rem wide, so in the image gallery the open menu hung 138px past the modal's right edge and gave the modal a horizontal scrollbar. Anchor it to the toggle's other edge instead.
|
Thanks for the pull request, @ihor-romaniuk! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3219 +/- ##
=======================================
Coverage 95.92% 95.92%
=======================================
Files 1397 1397
Lines 33581 33581
Branches 7947 7947
=======================================
Hits 32214 32214
Misses 1308 1308
Partials 59 59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
The "Add an image" gallery in the rich-text editors has two problems with its sort control.
Its label reads
Bynewest— the space between the "By" prefix and the sort option is missing, so every option renders asByoldest,ByName A-Zand so on. The label now reads "By newest" again.Opening that control made the whole modal scroll sideways: the menu is wider than the button it hangs from, so it spilled past the modal's right edge and the modal grew a horizontal scrollbar. The menu now stays inside the modal and the scrollbar is gone.
Both apply to the video gallery too, which uses the same control.
Implementation notes
SearchSort.jsx. dprint strips those between elements, and it did so in Use "dprint" as a code formatter, replacing eslint #2997 when it replaced eslint — runningdprint fmton the restored literal removes it again.{' '}survives formatting.SearchSorttests already match onBy oldest, but that is the accessible name, and the accname algorithm space-joins text across element boundaries, so they read "By oldest" from markup that displays "Byoldest". The new test asserts the rendered text instead.SelectMenuhard-codesplacement="bottom-start"and exposes no alignment prop, and its menu is--pgn-size-menu-item-width-base, 19rem, against a toggle of about 9rem. Measured in the running Studio: the menu ran to x=1258 while the modal ended at x=1120, and the modal'sscrollWidthexceeded itsclientWidthby 138px.!important. Popper's own transform stays 0,0 here, so the menu simply anchors to the toggle's other edge.Screenshot/Video
Testing
Verified locally
BynewesttoBy newestscrollWidth - clientWidthwent from 138px to 0; no element in the modal reports a horizontal scrollbardprint fmton the restored literal space{' '}survives — this is why the fix takes that formBynewest, passes after; the four existing tests pass either wayjeston SelectionModal, ImageUploadModal, VideoGallerynpm run lint,tsc --noEmit