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

1766 registration scroll #3791

Merged

Conversation

alondahari
Copy link
Collaborator

What? Why?

Closes #1766

Add collapsible snippets of information in the registration introduction modal so the action button is visible on smaller screens without scrolling.

What should we test?

  • Make sure modal is usable and intuitive on different size screens and orientations
  • Make sure it is obvious to a user that the collapse triggers are clickable

Release notes

  • Added collapsible information snippets for smaller screens on registration introduction modal

Changelog Category: Added

@luisramos0
Copy link
Contributor

nice, thanks for the PR!
can you share screenshots of the new page?

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

codeclimate warnings should be easy to fix.

Copy link
Member

@kristinalim kristinalim left a comment

Choose a reason for hiding this comment

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

This is great, @jazzdragon! 🙂 I just left a comment about some minor details.

= t(".registration_about_us")
%input#collapsible-registration-checklist.collapsible-checkbox{:type => "checkbox"}
%label.lbl-toggle{:for => "collapsible-registration-checklist"}
%h5= t(".registration_checklist")+":"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, a collapsed heading "You'll need:" doesn't look right... What do you think of changing the base translation to something similar to "What do I get?" in the second column, such as "What do I need?". To make it simple, it can be the same for both mobile and desktop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do this.

Copy link
Contributor

@luisramos0 luisramos0 Jun 13, 2019

Choose a reason for hiding this comment

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

%input#collapsible-registration-outcome.collapsible-checkbox{:type => "checkbox"}
%label.lbl-toggle{:for => "collapsible-registration-outcome"}
%h5
= t(".registration_outcome_headline")
Copy link
Member

Choose a reason for hiding this comment

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

Having this in a different line adds a space before the text, so the text is no longer aligned with the heading of the first set.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, there is the space in the html, on my test it looks aligned anyway.
I'll make this a single line.

Copy link
Contributor

@luisramos0 luisramos0 Jun 13, 2019

Choose a reason for hiding this comment

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

@luisramos0
Copy link
Contributor

moving to in dev.
only some minor changes required.

@luisramos0
Copy link
Contributor

@jazzdragon do you think you will pick this up again at some point?

@luisramos0 luisramos0 self-assigned this Jun 13, 2019
@luisramos0
Copy link
Contributor

I can finish this, just a few things to fix.

@luisramos0
Copy link
Contributor

This is how it looks, I think it's good!
iphone
Screenshot 2019-06-13 at 19 15 50

ipad
Screenshot 2019-06-13 at 19 17 05

desktop
Screenshot 2019-06-13 at 19 17 13

I am going to follow @kristinalim recommendation and change the label to "What do I need?"

@luisramos0 luisramos0 force-pushed the 1766-registration-scroll branch from 58282db to a370216 Compare June 13, 2019 18:28
@luisramos0 luisramos0 requested a review from kristinalim June 13, 2019 18:33
@luisramos0
Copy link
Contributor

ok, I've merged master into this branch, addressed Kristinas feedback, fixed rubocop issues and tested locally.
This is ready for review again.

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nice! I don't think we need to wait for Kristina. You addressed all her concerns.

@RachL RachL self-assigned this Jul 5, 2019
@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Jul 5, 2019
@RachL
Copy link
Contributor

RachL commented Jul 8, 2019

Windows 10 Edge 17:
image

Windows 10 Firefox 63:
image

MasOS Sierra Safari 10.1
image

Safari on iphone XS:
image

Safari on Ipad Mini
image

Chrome on Samsung Galaxy s10:
image

Chrome Sony Xperia:
image

Looking good, ready to go!

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Jul 8, 2019
@lin-d-hop
Copy link
Contributor

Strong testing game 💪

@luisramos0 luisramos0 merged commit fe71781 into openfoodfoundation:master Jul 8, 2019
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.

Enterprise Registration - Scroll on first step
6 participants