feat: Use dropdown for units when there are more than would fit in the UI #321
feat: Use dropdown for units when there are more than would fit in the UI #321xitij2000 wants to merge 2 commits intomitodl:mitx/teakfrom
Conversation
* feat: add canvas integration support (cherry picked from commit fdb818a)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @xitij2000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR, titled "feat: Use dropdown for units when more then 15", aims to improve the user experience in the sequence navigation when a course unit (sequence) contains a large number of items. Specifically, it introduces a dropdown menu to contain the list of units when the number of visible tabs exceeds a certain threshold based on the available width, preventing the navigation bar from becoming overly crowded and difficult to use.
Highlights
- UI Enhancement: The primary goal is to enhance the user interface for sequences with many units by collapsing overflowing units into a dropdown menu.
- Responsive Design: The implementation dynamically determines when to show the dropdown based on the width of the navigation container and the width of individual unit tabs, making the UI more responsive.
- JavaScript Logic: New JavaScript functions are added to handle rendering the dropdown, toggling its visibility, closing it when clicking outside, and re-rendering it on window resize.
- CSS Styling: CSS updates are included to style the new dropdown button and the list items within the dropdown, ensuring proper layout and appearance.
Changelog
Click here to see the changelog
- lms/templates/seq_block.html
- Added a new script template (
#dropdown-button-tpl) for the HTML structure of the dropdown button and container (lines 7-22). - Added the
sequence-list-itemclass to the list items representing individual units (line 66). - Added a conditional block to potentially include a
seq_dropdown.htmltemplate ifshow_dropdownis true (lines 118-120). Note: The include is currently commented out.
- Added a new script template (
- xmodule/js/src/sequence/display.js
- Added new prototype methods
toggleDropdown,renderDropdown, andhandleClickOutsideDropdownto theSequenceclass (lines 29-31, 44-48). - Initialized the dropdown button template and called
renderDropdownduring theSequenceconstructor (lines 74-75). - Implemented the
renderDropdownmethod to calculate overflow, add the dropdown button, hide overflowing tabs, and populate the dropdown list with clones of the hidden tabs (lines 80-103). - Updated the
bindmethod to attach event listeners for clicking outside the dropdown ($(document).click), clicking items within the dropdown (#dropdown-sequence-list .dropdown-item), clicking the dropdown button (#dropdown-sequence-list-button), and window resize ($(window).on('resize', _.debounce(this.renderDropdown, 200))) (lines 112-114, 120). - Implemented
handleClickOutsideDropdownto hide the dropdown when a click occurs outside its container (lines 123-126). - Implemented
toggleDropdownto simply toggle the display of the dropdown list (lines 129-131). - Added a line to hide the dropdown list at the beginning of the
gotomethod, ensuring it closes when a unit is selected (line 343).
- Added new prototype methods
- xmodule/static/css-builtin-blocks/SequenceBlockDisplay.css
- Adjusted the
max-widthfor the.sequence-list-wrapperto accommodate the dropdown button (line 81). - Added a media query to adjust the
max-widthfor larger screens (min-width: 768px) (lines 84-87). - Included
.dropdown-togglein the selector for list item styling (line 101). - Added a specific style rule for the
.dropdown-toggleheight within list items (lines 114-116). - Included
.dropdown-togglein the selector for button styling within list items (line 118). - Added new styles for list items and buttons within the dropdown container (
#dropdown-container ol li button) to control layout, padding, and text overflow for the unit title (lines 133-147).
- Adjusted the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Tabs fill up the bar,
Too many, they overflow,
Dropdown hides the rest.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a dropdown menu for sequence navigation tabs when there isn't enough space to display all of them. This is a useful enhancement for courses with many units in a sequence.
The implementation adds a new HTML template for the dropdown, corresponding JavaScript logic for rendering and interaction, and CSS for styling.
While the core idea is good, there are several critical and high-severity issues related to potential ID clashes with multiple instances, JavaScript errors (incorrect this context, global selectors, incorrect event targets), and accessibility concerns for the new dropdown. These should be addressed to ensure robustness and proper functionality.
Summary of Findings
- ID Clashes with Multiple Instances: The JavaScript code and HTML template use static IDs for dropdown elements. If multiple sequence blocks are on a page, these IDs will clash, leading to critical malfunctions. Selectors and IDs need to be instance-specific.
- JavaScript Context and Scoping Issues: Several JavaScript issues were found: incorrect
thiscontext in aneachcallback (high severity), use of global jQuery selectors instead of instance-scoped ones (high severity), and incorrect event target for window resize (high severity). - Accessibility of Custom Dropdown: The new dropdown component is missing key accessibility features, such as
aria-expandedattribute management, dedicated keyboard navigation for dropdown items, and proper focus management. This is a high-severity concern. - Potential JavaScript Errors: There's a potential for a division by zero error in
renderDropdownifsingleTabWidthis 0. Edge cases foroverFlowIdxalso need careful review. - CSS Best Practices: Use of
!importantin CSS and inline styles in HTML templates were noted, which could be improved for better maintainability.
Merge Readiness
This pull request introduces a valuable UI improvement. However, due to several critical and high-severity issues identified (particularly regarding ID clashes, JavaScript errors, and accessibility), I recommend that these changes not be merged until these issues are thoroughly addressed. Addressing these points will ensure the feature is robust, accessible, and functions correctly in all scenarios, especially when multiple sequence blocks are present on a page. I am unable to approve this pull request; please ensure these concerns are resolved and further review is conducted before merging.
| this.showCompletion = this.el.data('show-completion'); | ||
| this.keydownHandler($(element).find('#sequence-list .tab')); | ||
| this.base_page_title = ($('title').data('base-title') || '').trim(); | ||
| this.dropdownButtonTpl = _.template($('#dropdown-button-tpl').text())({}); |
There was a problem hiding this comment.
The template dropdown-button-tpl uses static IDs (e.g., dropdown-container, dropdown-sequence-list-button, dropdown-sequence-list). If multiple sequence blocks are rendered on the same page, these IDs will clash, leading to incorrect behavior (e.g., toggleDropdown affecting the wrong element, CSS styles applying incorrectly).
To handle multiple instances, consider:
- Generating unique IDs for each instance of the dropdown (e.g., by appending the sequence's unique ID).
- Using classes for JavaScript hooks and CSS styling primarily, and then using
this.$()to scope selections to the current sequence block's instance of the dropdown.
This is a critical issue if multiple sequence blocks can appear on one page.
| > | ||
| <span class="icon fa fa-chevron-down" aria-hidden="true"></span> | ||
| </button> | ||
| <div id="dropdown-sequence-list" style="display: none; position: absolute; width: 240px; right: 0;"> |
There was a problem hiding this comment.
The div#dropdown-sequence-list uses inline styles. For better maintainability and separation of concerns, could these styles be moved to a CSS class in SequenceBlockDisplay.css? This would also make it easier to manage styles consistently.
| .xmodule_display.xmodule_SequenceBlock .sequence-nav ol li .dropdown-toggle { | ||
| height: 49px !important; |
There was a problem hiding this comment.
The use of !important here for height: 49px !important; is generally discouraged as it can make CSS harder to maintain and override later.
Is it possible to achieve the desired height by increasing selector specificity or adjusting other conflicting styles, rather than relying on !important?
xitij2000
force-pushed
the
kshitij/sequence-ui-fix.mitx.teak
branch
3 times, most recently
from
4326ae4 to
d5c6c71
Compare
|
@xitij2000 we'd prefer to get this from upstream as well. Do you have a PR to backport it to teak? |
xitij2000
changed the title
xitij2000
changed the title
|
@pdpinch The upstream PR against master is here: openedx#36803 As with #320 there is no upstream backport to teak. |
When dealing with subsections that have a lot of units, show a dropdown for unit selection to simplify navigation. (cherry picked from commit aaca11f)
xitij2000
force-pushed
the
kshitij/sequence-ui-fix.mitx.teak
branch
from
d5c6c71 to
f7da073
Compare
3 participants
Description
Cherry pick of openedx#36803