Story 2421: Add Master and Develop Buttons to Library Page & Library Sub-page#2500
Story 2421: Add Master and Develop Buttons to Library Page & Library Sub-page#2500javiercoronadonarvaez wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds branch-switch button rendering for library pages and library subpages, including a new icon path, shared CSS, and template wiring for the page filter row and hero area. ChangesBranch buttons UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
javiercoronadonarvaez
force-pushed
the
javiercoronarv/add-master-develop-buttons
branch
2 times, most recently
from
50439db to
5b6861e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@templates/v3/includes/_branch_buttons.html`:
- Around line 24-30: The branch button template is passing
request.resolver_match.kwargs directly into branch_url_tag, and that tag mutates
the dict in place, so update the _branch_buttons.html calls to pass a copy
instead of the original kwargs. Use the existing branch_url_tag invocations for
the master and develop buttons as the fix point, ensuring any version_slug
changes stay isolated to the tag and do not leak into later template rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: beb8cac0-a9ab-489f-ac22-98e326efdfd5
📒 Files selected for processing (7)
static/css/v3/branch-buttons.cssstatic/css/v3/library-page.csstemplates/includes/icon.htmltemplates/v3/includes/_branch_buttons.htmltemplates/v3/includes/_hero_library.htmltemplates/v3/libraries/library-subpage.htmltemplates/v3/library_page.html
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Hi @javiercoronadonarvaez , this works great! I just left some nit comments around inline documentations, and I think the quick-win suggestion from CodeRabbit is also worth tackling – pre-approving now!
| /* | ||
| Branch Buttons | ||
| Small, unobtrusive Master ("M") / Develop ("D") branch links. | ||
| Bordered icon square with an amber corner badge. Matches Figma node 7696:45822. |
There was a problem hiding this comment.
Nit: Can we remove the Figma node reference?
| /* In the library sub-page hero the buttons are a direct child of __main | ||
| (sibling of the content column), pinned to the far right and bottom-aligned | ||
| with the actions row — matching Figma node 7146-55502 (hero is items-end). | ||
| The right edge is inset by --space-large so it lines up with the right edge | ||
| of the content cards below (.library-subpage-container has 16px side padding). | ||
| Figma node 7696-45623. */ | ||
| .hero-library__main > .branch-buttons { | ||
| align-self: flex-end; | ||
| flex-shrink: 0; | ||
| margin-right: var(--space-large); | ||
| } | ||
|
|
||
| /* Mobile: __main becomes a column, so the buttons stack below the actions row. | ||
| Left-align them with the cards' left edge (block + cards both pad 8px here) | ||
| and add a little separation. Figma node 7611-62006. */ | ||
| @media (max-width: 767px) { | ||
| .hero-library__main > .branch-buttons { |
There was a problem hiding this comment.
Nit: I think both of these comments are a little long, can we trim them down or remove them?
| {% block extra_head %} | ||
| {{ block.super }} | ||
| <link href="{% static 'css/v3/library-subpage.css' %}" rel="stylesheet"> | ||
| <link href="{% static 'css/v3/branch-buttons.css' %}" rel="stylesheet"> |
There was a problem hiding this comment.
Nit: Per our current convention, new component CSS goes in v3/components.css. Could you move this import there to keep things consistent? With that change, we should also be able to remove the import on the library_page.html
| V3 Branch Buttons — two small, unobtrusive links that switch the current page | ||
| to the Master / Develop Boost version, per Rob's approved design (Figma node | ||
| 7696:45822). | ||
|
|
||
| Each button is a bordered icon square with a small amber corner badge showing | ||
| a single letter ("M" / "D"). The full words "Master" / "Develop" are exposed | ||
| via title + aria-label for accessibility (the AC requires those exact words). |
There was a problem hiding this comment.
| V3 Branch Buttons — two small, unobtrusive links that switch the current page | |
| to the Master / Develop Boost version, per Rob's approved design (Figma node | |
| 7696:45822). | |
| Each button is a bordered icon square with a small amber corner badge showing | |
| a single letter ("M" / "D"). The full words "Master" / "Develop" are exposed | |
| via title + aria-label for accessibility (the AC requires those exact words). | |
| V3 Branch Buttons |
To me some of the detail in these 2 paragraphs seem a bit heavy and can easily go out-of-date (the Figma node ids plus the process/ticket context like "per Rob's approved design" and "the AC requires those exact words"). Can we consider trimming this down to just the non-obvious why that a future reader of the code needs (why the buttons are gated behind show_branch_buttons, why we swap version_slug) and dropping the node refs + approval/AC notes, since those live better in the PR/ticket and go stale once the design or scope moves? Does this seem reasonable to you?
javiercoronadonarvaez
force-pushed
the
javiercoronarv/add-master-develop-buttons
branch
from
c89f346 to
3ac9021
Compare
jlchilders11
left a comment
jlchilders11
left a comment
There was a problem hiding this comment.
Looks good, just a few things to get it ready to merge.
It looks like Julia caught everything that I would mention, but for me the Rabbit comment on mutating the kwargs and the location of the css are actual stoppers for approval. Once those are addressed happy to approve.
javiercoronadonarvaez
force-pushed
the
javiercoronarv/add-master-develop-buttons
branch
from
3ac9021 to
03683db
Compare
|
Thanks for all your suggestions, which were addressed in the last 3 commits. It is ready for another review. |
javiercoronadonarvaez
force-pushed
the
javiercoronarv/add-master-develop-buttons
branch
from
3747142 to
bc9d342
Compare
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
All looking good to me, thanks @javiercoronadonarvaez !
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Thanks for the updates @javiercoronadonarvaez ! This looks great 🙌
4 participants
Issue: #2421
Summary & Context
Adds small, intentionally unobtrusive Master ("M") / Develop ("D") branch buttons to the library page and library sub-page, per Rob's approved design. The buttons switch the on-site Boost version (master/develop) — the original ticket scoped these as navbar version-dropdown entries, but the approved direction is two standalone buttons instead.
localhost:8000/libraries/latest/grid/Changes
git-branchpixel-art icon to the shared icon component (templates/includes/icon.html).templates/v3/includes/_branch_buttons.htmlrendering the M/D buttons. It is page-agnostic — URLs are derived from the current view via the existingbranch_url_tagtemplate tag (swaps theversion_slug), so the same partial works on the list and detail pages. Full words "Master"/"Develop" are exposed viatitle+aria-label.static/css/v3/branch-buttons.css(button styling using design tokens, plus the sub-page hero placement/alignment rules for desktop and mobile).templates/v3/library_page.html+static/css/v3/library-page.css): buttons placed at the far right of the filter toolbar row (.library-page__filter-row).templates/v3/libraries/library-subpage.html+templates/v3/includes/_hero_library.html): buttons rendered in the hero as a direct child of__main, far-right and bottom-aligned with the actions row (right edge aligned to the content cards below; left-aligned and stacked on mobile). Gated behind a newshow_branch_buttonsflag so the four other pages that share_hero_library.html(community, release, examples) do not render them.Please list any potential risks or areas that need extra attention during review/testing
branch_url_tag→ thelibraries-list/library-detailviews →set_selected_boost_version. Verified that navigating sets/clears theboost_versioncookie (master/developset it,latestclears it)._hero_library.htmlis used by community / release / examples pages too — the buttons are gated behindshow_branch_buttons(passed only from the sub-page). Worth confirming the buttons appear only on the two intended pages.title/aria-label. Confirmed as the convention going forward.Screenshots
Library Page Functionality
LibraryPageFunctionality.mov
Library Page Responsiveness and Modes
LibraryPageResponsivenessAndModes.mov
Library Sub Page Functionality
LibrarySubPageFunctionality.mov
Library Sub Page Responsiveness and Modes
LibrarySubPageResponsivenessAndModes.mov
Self-review Checklist
Frontend
Summary by CodeRabbit
:focus-visiblekeyboard focus outlines.aria-labeltext,role="group"for the switch control, and anaria-hiddendecorative badge letter.