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

D8CORE-105: Local footer. #510

Merged
merged 93 commits into from
Oct 23, 2019
Merged

D8CORE-105: Local footer. #510

merged 93 commits into from
Oct 23, 2019

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Oct 9, 2019

READY FOR REVIEW

Summary

  • Creates a responsive local footer component
  • Adds FA assets
  • Adds a link list macro
  • Adds signup form component
  • Updates Dependencies
  • Adds tugboat

Review By (Date)

  • Please review by October 18

Urgency

  • Med-high as we would like to get it deployed to our pilot at the end of the month

Steps to Test

  1. Check out this branch
  2. Update npm dependencies npm install
  3. Compile code npm run build
  4. Compile styleguide npm run styleguide
  5. Review code
  6. Compare to mock: https://www.figma.com/file/Kmd4utmJFPRMVeCFEEBQhLtx/Stanford-Design-Library?node-id=5268%3A11637
  7. Reivew for accessibility

Affected Projects or Products

  • Decanter
  • D8 Core project

Associated Issues and/or People

See Also

@yvonnetangsu
Copy link
Member

FYI @sherakama , JB said we probably want to check with Dana Granoski, our brand designer on the local footer design first.

@sherakama
Copy link
Member Author

@kerri-augenstein Heads up.

@sherakama
Copy link
Member Author

sherakama commented Oct 10, 2019

@yvonnetangsu On a slightly different tangent, I figured out a way to get FA into this PR without adding ALL of the markup. I did have to add the fonts to the default decanter.css assets but nothing shows up in the decanter-no-markup which is good. And we have access to their sass mixins/vars/etc.

@yvonnetangsu
Copy link
Member

@yvonnetangsu On a slightly different tangent, I figured out a way to get FA into this PR without adding ALL of the markup. I did have to add the fonts to the default decanter.css assets but nothing shows up in the decanter-no-markup which is good as we have access to their sass mixins/vars/etc.

Sounds great - thanks, @sherakama !

@yvonnetangsu
Copy link
Member

Quick comments -

  1. You're so fast! 😄
  2. Perhaps add a gap between the two navs (in the middle cell) that's the same width as the grid gap. So if some links get long, they don't run into each other.
  3. For prefooter, we keep the font weight on links 400 (Regular) except for the action links - though we could wait to change these things till Kerri finalize things with Dana.
  4. I have some code written for the social media icons that I could share (JB wants them to show brand color when hovered over).

I'll do a more in-depth review soon.

@yvonnetangsu
Copy link
Member

And it looks like the action link in the local footer has a different icon than the one in the link component section? Should we add that as a link variant?

@yvonnetangsu
Copy link
Member

// Social media brand colors
$social-brands: (
    'Facebook':                    #3b579d,
    'Twitter':                     #1da1f2,
    'Instagram':                   #d73676,
    'LinkedIn':                    #0077b5,
    'YouTube':                     #cd201f,
    'iTunes U':                    #a3aaae,
    'Reddit':                      #ff5700,
);

I copied the above from our WP theme's color map - please feel free to use them for the local footer. We use them as hover/active colors. Icon normal states are black.

@yvonnetangsu
Copy link
Member

yvonnetangsu commented Oct 11, 2019

@sherakama , seems like the headings "links to", "resources for", "Sign up for our email" could be h2 instead of <p> so the voice over rotor can list them as headings?

@kerri-augenstein, in the Figma design, "Sign up for our email" has larger font size than "links to" and "resources for". Is it possible to make them the same (maybe the larger size)?

I talked to JB and we're modifying the WP theme to use the local footer design. However, using our WP template, site builders can put in custom content and widgets, and they each can have a heading - so I'm wondering what's the official "heading" style for the local footer.

@kerri-augenstein
Copy link

kerri-augenstein commented Oct 11, 2019

@yvonnetangsu @sherakama yep, I hear ya on the headers.

Let's modify all of them to be 18px. No need for any changes on any breakpoints.

And we can make this change across every option for blocks' headings, so they are all consistent.

Designs are updated
https://www.figma.com/file/Kmd4utmJFPRMVeCFEEBQhLtx/Stanford-Design-Library?node-id=5268%3A11637

@sherakama
Copy link
Member Author

And it looks like the action link in the local footer has a different icon than the one in the link component section? Should we add that as a link variant?

I am using the link-icon mixin for this element.

@yvonnetangsu
Copy link
Member

Quick question about the lockup in the local footer - is it only going to display line 1 regardless of how many lines the full lockup in the header is for the department?

@kerri-augenstein
Copy link

@yvonnetangsu great question! To keep it simple, we would mirror whatever lockup is present in the header. So it would include any text/styles/lines. But. There is no reason you would/should need to include this in your Wordpress platform if your sites don't require the lockup.

@sherakama
Copy link
Member Author

sherakama commented Oct 22, 2019

@kerri-augenstein @yvonnetangsu

  1. I left the login button as rounded.
  2. I put the two lists of links as side x side on the MD breakpoint
  3. Headers are all 18px now for all breakpoints

I have also abstracted the signup-form into its own component as Kerri has identified projects where it will be used in multiple places. I'll work on the moving placeholder text for the form next. IE 11 fixes are in place. The silly autoprefixer got the grid helper settings wrong and needed to be helped out.

@sherakama
Copy link
Member Author

Looks like moving placeholders will take a bit of work. I'm going to create a ticket for it so that it doesn't block this PR.

@sherakama
Copy link
Member Author

#519

@yvonnetangsu
Copy link
Member

Thanks for doing this, @sherakama . I'll find some time to find Kevin's code for the signup form label soon. Should I push to this branch directly?

@yvonnetangsu
Copy link
Member

@sherakama sorry, please ignore above comment. I misunderstood placeholder as sass placeholders 😄 . Shall I push the placeholder/label code to the new branch once you set it up?

@sherakama
Copy link
Member Author

I would prefer to have this merged sooner than later so if I could pick I would ask that you spend your time on review first and then we get to the moving placeholders. I'm happy to move this work to the v6 branch if you don't feel that it meets our accessibility standards. IMHO, the moving placeholder text is an improvement and not an accessibility blocker.

@yvonnetangsu
Copy link
Member

@sherakama , this is looking really great - about perfect on IE except that white space that doesn't take on the fog color background on the right, but I'm fine with it since it's IE. Responsive behavior, heading sizes look great. There are some weird issues with Edge 18 but looks like .kss-fullscreen-mode need a width:100%; and that would solve the problem.

I'm thinking that some of the remaining design related tweaks (button styles, link colors) can be worked on in another PR after we hear Dana/Kerri's final decisions.

That leaves the only thing that's on my mind - currently the normal font size for the local footer just follows the body font size (19px on desktop), but I believe Kerri has it a bit smaller for the local footer. I'm also fine with finetuning this in another PR if you're eager to merge this.

@sherakama sherakama temporarily deployed to Tugboat October 23, 2019 17:02 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 23, 2019 19:32 Destroyed
@sherakama
Copy link
Member Author

@yvonnetangsu

Ok, I made a few font-size related tweaks, but if we could say this is good enough, I'll change the base branch to v6 and we can merge it in to there as we wait for the feedback on buttons and link colors.

@sherakama sherakama changed the base branch from master to v6 October 23, 2019 19:33
@sherakama sherakama temporarily deployed to Tugboat October 23, 2019 19:39 Destroyed
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

I'm approving this for now, but I do believe we'll need to finetune the font sizes in another PR. We can bundle all those tweaks there. Thanks for working on this!

@sherakama sherakama merged commit 102dc37 into v6 Oct 23, 2019
@sherakama sherakama deleted the D8CORE-105 branch October 23, 2019 20:38
@sherakama
Copy link
Member Author

Thank you so much for all of the review!

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

Successfully merging this pull request may close these issues.

4 participants