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

[Part 2 of 3] Newsletter sign up button/form added newsletter button #3048

Merged
merged 31 commits into from
Jul 25, 2019

Conversation

youriwims
Copy link
Contributor

@youriwims youriwims commented Apr 22, 2019

Related PRs/issues #2994

https://foundation-mofostaging-pr-3048.herokuapp.com/

Checklist

  • Add Newsletter button
  • Mobile: Donate stays put while Newsletter button moves down below nav items
  • Clicking on Newsletter button animates sign up form to open and page content slides down
  • Clicking anywhere outside of the black form box animates the form to close
  • Form error/success/hover states should follow footer styles

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3048 April 22, 2019 21:22 Inactive
@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3048 April 26, 2019 21:19 Inactive
@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3048 April 29, 2019 16:29 Inactive
@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3048 April 29, 2019 16:34 Inactive
@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3048 May 8, 2019 06:39 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 8, 2019 17:22 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 8, 2019 23:19 Inactive
@mmmavis mmmavis changed the title [Part 2 of 3] Newsletter sign up button/form added newsletter button (WIP) [Part 2 of 3] Newsletter sign up button/form added newsletter button May 8, 2019
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 8, 2019 23:21 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 9, 2019 00:07 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 9, 2019 17:00 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 9, 2019 17:02 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 9, 2019 21:45 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 9, 2019 21:51 Inactive
@mmmavis mmmavis marked this pull request as ready for review May 9, 2019 22:22
@mmmavis
Copy link
Collaborator

mmmavis commented May 9, 2019

@kristinashu this is ready for design review. Note that the desktop version form is still in one-column layout. I'm working on modifying the Newsletter/Sign Up React component so we can have the option to show it as 2-column. I'm separating the work from this PR as that React component is used in other places (e.g., footer) and it's best that we do it as 2 stages in case I accidentally break something.

Another note is that I can't perfectly vertically centered the form section. We have to reserve some extra space for error messages to go. (Technical context: in order to achieve the slide down effect, we have to give the fully expanded section a fixed height before the animation starts so the slide down animation knows when to 'stop growing/expanding'. If we really want it to look centered of course we can go with some JavaScript help, but hopefully we can do this as a follow-up ticket.)

Artboard 1

@mmmavis mmmavis changed the title (WIP) [Part 2 of 3] Newsletter sign up button/form added newsletter button [Part 2 of 3] Newsletter sign up button/form added newsletter button May 9, 2019
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 May 15, 2019 20:26 Inactive
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3048 July 17, 2019 05:55 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 July 17, 2019 06:49 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 July 18, 2019 21:23 Inactive
@mmmavis
Copy link
Collaborator

mmmavis commented Jul 18, 2019

@kristinashu this is up for design review again!

Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

Beautiful!!!

@Pomax
Copy link
Contributor

Pomax commented Jul 24, 2019

It looks like the button stays highlighted if you click it once to open the dialog, and then click it again to dismiss the dialog. Should it reset to outline styling?

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3048 July 24, 2019 21:30 Inactive
source/js/main.js Outdated Show resolved Hide resolved
source/js/nav-newsletter.js Outdated Show resolved Hide resolved
source/js/primary-nav.js Outdated Show resolved Hide resolved
source/js/nav-newsletter.js Outdated Show resolved Hide resolved
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 July 25, 2019 06:14 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3048 July 25, 2019 06:18 Inactive
@mmmavis
Copy link
Collaborator

mmmavis commented Jul 25, 2019

It looks like the button stays highlighted if you click it once to open the dialog, and then click it again to dismiss the dialog. Should it reset to outline styling?

@Pomax I can't reproduce the issue. Maybe your had your cursor over the button that's why it stayed highlighted?

newsletter-btn-highlighted

@mmmavis mmmavis requested a review from Pomax July 25, 2019 06:22
@Pomax
Copy link
Contributor

Pomax commented Jul 25, 2019

Both in FF and Chrome the button stays black for me.

STR:

  • load https://foundation-mofostaging-pr-3048.herokuapp.com/en/
  • move mouse to over "newsletter button" => button turns black
  • click button => newsletter dialog comes up
  • click button again => newsletter dialog disappears, button stays focused (focus-outline shows)
  • move mouse away from button => button remains focused (focus-outline shows) and filled black until you focus on a different part of the page, or remove focus by clicking an inactive part.

Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

R+ after fixing the prettier errors - the button coloring probably shouldn't block this from landing so fix if you feel that's easy to do, otherwise landing it works for me!

@mmmavis
Copy link
Collaborator

mmmavis commented Jul 25, 2019

OMG FINALLY! THANKS TEAM!

@mmmavis mmmavis merged commit 59427d2 into master Jul 25, 2019
@mmmavis mmmavis deleted the newsletter-button-form branch August 14, 2019 19:07
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.

None yet

6 participants