SDSS-595 | SDSS-596: Add Newsroom menu

[joegl]

READY FOR REVIEW

Summary

• adding a newsroom menu.
• Figma location: https://www.figma.com/file/B3C3AdKmu6HmyUoicNwoVv/SDSS-Components-Library?type=design&node-id=2925%3A19359&mode=design&t=qX8JN0GLE9uuCUnJ-1

Review By (Date)

• 8/30

Criticality

• High

Review Tasks

Setup tasks and/or behavior to test

1. Check out this branch

2. You have to set up the menu and add links

3. Go to this menu, /admin/structure/menu/manage/newsroom and add links per the design. The links are added in columns.

4. Go to block, /admin/structure/block and add the News & Research menu to the block.

5. Go to the menu block, /admin/structure/block/manage/newsresearch?destination=/admin/structure/block and limit the pages for the menu to be displayed on.

7. Verify you get the newsroom menu and it open closes. verify you can tab through the pages.
8. Verify the esc works for the closing the menu.
9. This is what the menu link looks like when configuring.

Front End Validation

• Is the markup using the appropriate semantic tags and passes HTML validation?
• Cross-browser testing has been performed?
• Automated accessibility scans performed?
• Manual accessibility tests performed?
• Design is approved by @ user?

Backend / Functional Validation

Code

• Are the naming conventions following our standards?
• Does the code have sufficient inline comments?
• Is there anything in this code that would be hidden or hard to discover through the UI?
• Are there any code smells?
• Are tests provided? eg (unit, behat, or codeception)

Code security

• Are all forms properly sanitized?
• Any obvious security flaws or new areas for attack?

General

• Is there anything included in this PR that is not related to the problem it is trying to solve?
• Is the approach to the problem appropriate?

Affected Projects or Products

• Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

• SDSS-595
• SDSS-596

Resources

• AMP Tool
• Accessibility Manual Test Script
• HTML Validator
• Browserstack and link to Browserstack Credentials

[jdwjdwjdw]

Thanks @jenbreese! Overall I really like how things are looking. I did identify some issues/questions, which I have addressed below.

• I'm not sure how relevant this is, but when I spun this PR up and went to /admin/structure/block/ there were already two News & Research blocks (one at /admin/structure/block/manage/newsroom?destination=/admin/structure/block/list/sdss_subtheme and one at /admin/structure/block/manage/newsresearch?destination=/admin/structure/block/list/sdss_subtheme) added to the Newsroom Menu block. I was confused why there were two blocks already there and figured it'd be good to point out. I also had to add /news to the page visibility configuration in order for it to display on the news page
  

• Desktop

  • I think the vertical alignment between the News & Research, links, and search input are a little off. It'd be nice if the links and search were all vertically aligned closer to the center of News & Research.
    

  • Additionally, I think it'd be good if the dropdown icons were moved down a little bit to better align with the links
    

  • When the links and search input are squashed into two lines, the dropdown menu covers up some of the News & Research headline and the search input

    • Around this same breakpoint, the last two link buttons in the dropdown look a little funky with a lot of extra bottom spacing
      

• Mobile

  • The main-menu search icon has some positioning issues while signed out
    

  • The sub-menu search input looks like it could use some spacing between the icon and the text
    

  • Mobile drop-down icons

    • It looks like in Figma these icons have a circle border and some color changes on hover. Is that feasible?
      

    • The dropdown icons have a border bottom applied all of the time, is that intentional?
      

    • When I tab through the sub-menu, there is no focus indicator while tabbing through the icons

  • When I tab through the mobile menu, it doesn't close once you reach the bottom of the links. I think this is an a11y issue, as it was something we had difficulty getting right for ADAPT in the past

  • It looks like the sub-menu breaks if there are too many links in the menu. Here is how it looks when you open the Browse topics sub-menu on mobile when it has all of the links added
    

  • Should the last two links the mobile sub-menu also look like buttons, to match figma?

  • From figma, when you open a submenu dropdown (e.g. Browse topics), it looks like the Browse topics div should change colors as well, not stay the same color)
    

Let me know if you have any questions / need any clarifications!

[jdwjdwjdw]

This is causing issues with the search icon placement on mobile while signed out

[joegl]

@jenbreese I can quickly help with the double block placement. I see two block configs for the news menu in this PR:

• docroot/profiles/sdss/sdss_profile/config/sync/block.block.newsroom.yml
• docroot/profiles/sdss/sdss_profile/config/sync/block.block.newsresearch.yml

These appear to be duplicates of one another and you only need one in there.

[joegl]

Also thanks @jdwjdwjdw for the thorough review!

[jenbreese]

Also thanks @jdwjdwjdw for the thorough review!

I so appreciate working with @jdwjdwjdw He's awesome

[jenbreese]

@jdwjdwjdw I fixed the following issues:

1. Extra block
2. Desktop issues
3. The first two mobile issues.

[jenbreese]

Update:

1. Search needs addressing when not logged in.
2. The underlines were removed.
3. added the circle but needs adjustment.
4. fixed the green background on mobile. I tested on my iphone/safari and desktop browsers.

[jenbreese]

Fixed items:

1. Should the last two links the mobile sub-menu also look like buttons, to match figma?
2. From figma, when you open a submenu dropdown (e.g. Browse topics), it looks like the Browse topics div should change colors as well, not stay the same color)
3. It looks like the sub-menu breaks if there are too many links in the menu. Here is how it looks when you open the Browse topics sub-menu on mobile when it has all of the links added

[jdwjdwjdw]

All of the issues that I identified have been addressed, and everything is looking great as far as I can tell 🚀