Expandable/dropdown menus #3917
Conversation
…aching-app into expandable-menus
| def sync_mode(mode) | ||
| mode == :mobile ? :desktop : :mobile | ||
| end |
There was a problem hiding this comment.
this seems slightly counterintuitive at first glance
There was a problem hiding this comment.
maybe there's a way of expressing what sync means in this context
There was a problem hiding this comment.
yep I think I could phrase that better 👍
app/views/pages/browse.html.erb
Outdated
| <!-- The <ul> element is dynamically generated --> | ||
| <!-- //NOSONAR --> |
| // fires when the user clicks on a navigation menu item | ||
| const item = event.target.closest('li'); | ||
|
|
||
| if (item) { |
There was a problem hiding this comment.
suggest avoid nesting with if (!item) return;
| const childMenuSync = this.getTargetItem(item.dataset.childMenuSyncId); | ||
| const toggleSecondaryNavigation = item.dataset.toggleSecondaryNavigation; | ||
|
|
||
| if (!directLink) { |
There was a problem hiding this comment.
again i think better to return rather than nest
| } | ||
|
|
||
| getTarget(id) { | ||
| if (id) { |
| } | ||
|
|
||
| toggleIconExpanded(item) { | ||
| if (item) { |
There was a problem hiding this comment.
if (!item) return false;
| toggleIconExpanded(item) { | ||
| if (item) { | ||
| const icon = item.querySelector('span.nav-icon'); | ||
| if (icon) { |
There was a problem hiding this comment.
if (!icon) return false;
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
you could possibly factor out a shared method here, eg
toggleIcon(item, expand = true) {
if (!item) return false;
const icon = item.querySelector('span.nav-icon');
const linkOrButton = item.querySelector('a, button');
if (!icon || !linkOrButton) return false;
const currentClass = expand ? 'nav-icon__contracted' : 'nav-icon__expanded';
const newClass = expand ? 'nav-icon__expanded' : 'nav-icon__contracted';
const ariaExpanded = expand ? true : false;
if (icon.classList.contains(currentClass)) {
icon.classList.replace(currentClass, newClass);
item.classList.toggle('selected', expand);
linkOrButton.ariaExpanded = ariaExpanded;
return true;
}
return false;
}
which is used by
toggleIconExpanded(item) {
return this.toggleIcon(item, true);
}
toggleIconContracted(item) {
return this.toggleIcon(item, false);
}
|
|
||
| context "when using a desktop browser" do | ||
| it "renders a dropdown menu for category links" do | ||
| expect(page).to have_css("#secondary-navigation > div.category-links > ol.category-links-list > li[id='page-five-category-1-desktop'] > button") |
There was a problem hiding this comment.
is there a less brittle and more readable way to test these things? eg
within('#secondary-navigation') do
within('div.category-links') do
expect(page).to have_button('whatever the button text is')
end
end
There was a problem hiding this comment.
can't seem to make this approach work? - the within blocks are skipped over
|
LGTM! Just a few suggestions |
Co-authored-by: Carlos Martinez <carlosjmtz@gmail.com>
…aching-app into expandable-menus
|
Review app deployed to https://get-into-teaching-app-review-3917.test.teacherservices.cloud |
|
Trello card
Expandable/dropdown menus
Context
Changes proposed in this pull request
New dropdown menu system for mobile and desktop
Guidance to review
Please test at multiple screen resolutions (mobile, tablet and desktop)