Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

[CR-86] - added an ability to show a third level of main menu for the mobile screen #2274

Open
wants to merge 1 commit into
base: 9.x-2.x
Choose a base branch
from
Open

[CR-86] - added an ability to show a third level of main menu for the mobile screen #2274

wants to merge 1 commit into from

Conversation

aleevas
Copy link
Contributor

@aleevas aleevas commented Nov 3, 2020

This PR should fix an issue with third level of menu for mobile screen.
How it looks now:
image-2020-11-03-13-50-27-782

How it will looks after fix:
CR-86ThirdLevelMenu

CR-86ThirdLevelMenu2

Steps for review

  • Log as admin
  • Set the Carnation theme as default.
  • Please add a third level item to the main menu, if you don't have it still.
  • Checks from mobile device, you should see a third level of items in mobile main menu.

General checks

  • All coding styles are fulfilled and there are no any issues reported by CodeSniffer. See Code of Conduct.
  • Documentation has been updated according to PR changes.
  • Steps for review have been provided according to PR changes.
    Steps for review
  • Make sure you've provided all necessary hook_update_N to support upgrade path.
  • Make sure your git email is associated with account on drupal.org, otherwise you won't get commits there.
    drupal.org email
  • If you would like to get credits on drupal.org, check documentation.

Thank you for your contribution!

@gundevel
Copy link
Collaborator

gundevel commented Nov 3, 2020

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

@hamrant
Copy link
Contributor

hamrant commented Nov 3, 2020

Can we make something similar to YGBW mobile menu?

  1. The second level link also can be expandable and have icons
  2. I think should be more difference between second and third level (at least biggest padding from left, maybe font size)

Screenshot from 2020-11-03 17-44-43

@podarok what do you think?

@hamrant hamrant added Five Jars Assigned for resolution PR: Needs Review Needs someone review ( code ) PR: Needs Testing Manual testing is needed labels Nov 3, 2020
@ddrozdik ddrozdik added the PR: Needs Work Unfinished task. Issues still there label Nov 4, 2020
@podarok podarok added the Type: Enhancement Change request. New functionality. label Nov 4, 2020
podarok
podarok suggested changes Nov 4, 2020
View reviewed changes
themes/openy_themes/openy_carnation/templates/menu/mobile/menu--main--mobile-menu.html.twig
@@ -60,9 +60,33 @@
{% for item in items %}
{% set ia = item.attributes.addClass(['nav-item nav-level-3 menu-item-' ~ item.title|clean_class]) %}
<li{{ ia }}>
{{ link(item.title, item.url, {'class' : 'nav-link'}) }}
{% if item.below %}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should go to all 3 themes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@podarok this one already present in templates for others theme, but was hidden by css.
Now I've fixed it and added some visual improvements for it.
So it should looks like:
Carnation:
CR-86CarnationMobileMenu

Rose:
CR-86RosyMobileMenu

Lily:
CR-86LilyMobileMenu

Please check does it good enough?

cc @hamrant

Copy link
Contributor

Choose a reason for hiding this comment

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

@aleevas Looks ok to me. Can we make the second level collapsible too?

@podarok
Copy link
Contributor

podarok commented Nov 4, 2020

@hamrant due to this is an enhancement - we should

  • populate change to all 3 themes
  • stick close to real user's demand

/cc @sarah-halby

@gundevel
Copy link
Collaborator

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

5 similar comments
@gundevel
Copy link
Collaborator

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

@gundevel
Copy link
Collaborator

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

@gundevel
Copy link
Collaborator

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

@gundevel
Copy link
Collaborator

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

@gundevel
Copy link
Collaborator

Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution.

@podarok podarok changed the base branch from 8.x-2.x to 9.x-2.x April 29, 2022 14:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hamrant hamrant hamrant left review comments

@podarok podarok podarok requested changes

At least 1 approving review is required to merge this pull request.

Assignees

@aleevas aleevas

Labels
Five Jars Assigned for resolution PR: Needs Review Needs someone review ( code ) PR: Needs Testing Manual testing is needed PR: Needs Work Unfinished task. Issues still there Type: Enhancement Change request. New functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aleevas @gundevel @hamrant @podarok @ddrozdik