Skip to content
This repository was 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
aleevas wants to merge 1 commit into
ymcatwincities:9.x-2.xfrom
aleevas:CR-86
Open

[CR-86] - added an ability to show a third level of main menu for the mobile screen#2274
aleevas wants to merge 1 commit into
ymcatwincities:9.x-2.xfrom
aleevas:CR-86

Conversation

@aleevas
Copy link
Copy Markdown
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!

@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins 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
Copy Markdown
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
Comment thread themes/openy_themes/openy_carnation/templates/menu/mobile/menu--main--mobile-menu.html.twig
{% 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
Copy Markdown
Contributor

@podarok podarok Nov 4, 2020

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
Copy Markdown
Contributor Author

@aleevas aleevas Nov 5, 2020

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
Copy Markdown
Contributor

@hamrant hamrant Nov 5, 2020

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
Copy Markdown
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

@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins commented Mar 15, 2022

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
@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins commented Mar 15, 2022

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

@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins commented Mar 15, 2022

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

@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins commented Mar 15, 2022

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

@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins commented Mar 15, 2022

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

@bjwilkins
Copy link
Copy Markdown
Collaborator

bjwilkins commented Mar 15, 2022

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

2 more reviewers

@hamrant hamrant hamrant left review comments

@podarok podarok podarok requested changes

Reviewers whose approvals may not affect merge requirements

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 @bjwilkins @hamrant @podarok @ddrozdik