Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/61 limit utility nav #284

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Conversation

alana29s
Copy link
Contributor

@alana29s alana29s commented Oct 4, 2021

Fixes #61 .

Changes proposed in this pull request

  • Limit the output hierarchy depth to the parent level.
  • Add an admin notice stating the limitations of the Utility Menu only on the Utility menu screen.
  • Add a function to reset nested utility nav menu items to the parent level.
  • Add an error notice informing the users that nested menu items have been reset to the parent level.
  • Add Javascript functionality preventing users from nesting a menu item, by reseting nested items to the parent/top level after dropped, on page load, and notifying the user view alert, for each nested item.

Important

The original request was for a single site. This update affects all responsive-framework-2x themes and child themes. If sites have nested utility items they will disappear from view immediately on launch of this update.

Additionally, if a user attempts to update the menu, this update will reset all nested menu items to the parent level and they will then display as top level items.

Based on the recommendation in issue #61 no analysis was completed to determine the impact of any existing sites.

Testing

  1. Test as site that has nested utility menu items before this update. Update the theme and watch the behavior. The items will not display upon immediate release of this update. When the user edits the utility nav, all nested items will be reset to the parent level, and on save updated to the parent level in the database. Then all items at the parent level will be displayed.
  2. Test the update has no affect on other menus in the Menu Update admin screen and the output.
  3. Test there is no impact to the output of a site without nested utility menu items.

Test Sites

Review checklist

  • I have tested my changes in my sandbox on Responsive Framework and at least one child theme.
  • I have tested any new filters or action hooks I have introduced in a child theme to ensure they work correctly.
  • I've reviewed the contribution guidelines.
  • I've updated CHANGELOG.MD with a brief explanation of the changes in this pull request in the unreleased section.
  • My code follows BU Coding Standards.

@alana29s alana29s self-assigned this Oct 4, 2021
Copy link
Contributor

@ashleykolodziej ashleykolodziej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zoom_0.mp4

Can we make it so that the nav item isn't removed when you finish the drag and drop? Details in the video above.

Copy link
Contributor

@hirozed hirozed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@alana29s
Copy link
Contributor Author

@ashleykolodziej I've updated the behavior to reset the menu position to the top/parent level instead of deleting the item on drop.

Updating on the drop reseting the base position was an easy adjustment.

I'm unsure if I can prevent an item from being nested at this time. I could constantly update the class as an item is in motion but that may cause a lot of JS memory/processing to be used if the process continuously loops while an item is being moved around.

If you'd like me to explore the prevention option I can queue that up hopefully for sometime next week or the week after.

@ashleykolodziej
Copy link
Contributor

@stonybrookweb I just tested this out and I think we might need to explore either prevention or a way to send updated data to WordPress - it seems like the class is not enough to do that, so the behavior when you save and update is a little funky. WordPress eventually ends up removing the nav item:

zoom_0.mp4

Let me know what you think - this seems to be a pretty tricky problem. I really like where the interaction and feedback is going though if we can get this figured out.

…ting the items.

Update Alert message.
Update Notice.
add i18n
update comments
@alana29s
Copy link
Contributor Author

@ashleykolodziej I've updated the whole methodology to reset nested items to the parent level, no more deletions. This is on the database level if an item gets through and on the JS level to reset after drop. There is a hidden span .is-submenu that exists for each menu item that is toggled by the WordPress Menu JS, so I was able to leverage that toggle. I've updated the messaging and provided some ideas for testing. From my testing, this seems to be a good workable solution.

Additionally wordsmithing may be helpful to create more user centric messages.

I look forward to your feedback!

@ashleykolodziej
Copy link
Contributor

I think this is functionally working exactly like what I'd expect now. I'm trying to think of a more user-friendly way to handle the notification that the nested item is not allowed and will be reset to the top than an alert. This is happening in JavaScript, right? Maybe I can add in a small animation or color hint or something so it's not an alert. Since it's so easy to accidentally nest a sub-item, I think an alert could quickly get annoying for users.

@stonybrookweb Would it be alright if I branched off this and tried a couple of UI approaches to finalize this? I think your work is great here and doesn't need changes, just want to get that last interaction feeling really good before we send it out to the world. :D

Copy link
Contributor

@ashleykolodziej ashleykolodziej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally approved, but I think I should take a crack at the UI here. Can you hold on merging until I submit that pull request?

@alana29s
Copy link
Contributor Author

@ashleykolodziej Yes, I'll hold off until you determine a better UI/UX to inform the user. Let me know if you have any questions or need any help or ideas. Since the update already loops through all the menu items, it would be simple to add a class instead. An idea could be to put a text on the item, that is removed via an animation time out in css a few seconds after it is reset.

@alana29s
Copy link
Contributor Author

@ashleykolodziej checking in to see if you need any additional help or information from me to work on the UI.

cc: @sr4136 @smtierneybu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit utility navigation output, do not nest
3 participants