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

GH-2032 Style Forgot Password in Hub #535

Merged
merged 10 commits into from Apr 28, 2020
Merged

GH-2032 Style Forgot Password in Hub #535

merged 10 commits into from Apr 28, 2020

Conversation

@benstrumeyer
Copy link
Contributor

@benstrumeyer benstrumeyer commented Apr 27, 2020

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?
  • Change the styling of the Forgot Password view to match other hub views

Ticket: https://cliqztix.atlassian.net/browse/GH-2032

@benstrumeyer benstrumeyer requested a review from Eden12345 Apr 27, 2020
@benstrumeyer benstrumeyer requested review from wlycdgr and ghostery/ghostery as code owners Apr 27, 2020
@benstrumeyer benstrumeyer self-assigned this Apr 27, 2020
Copy link
Contributor

@Eden12345 Eden12345 left a comment

Could you center the Forgot Password form vertically to match the other views? It is currently positioned at the top of the page.

Also, as you resize the window horizontally, the horizontal positioning starts to get funky:
Screen Shot 2020-04-27 at 3 58 49 PM

And on the smallest screen, you can't see the submit button and the header copy gets cut off:
Screen Shot 2020-04-27 at 4 01 27 PM

The Forgot Password form should match the other forms in the Hub, which are centered and in full view no matter which window size you are using.

@@ -46,7 +46,7 @@ const Hub = () => (
<Route exact path="/rewards" component={RewardsView} />
<Route exact path="/products" component={ProductsView} />
<Route exact path="/create-account" component={CreateAccountView} />
<Route exact path="/forgot-password" render={() => <ForgotPasswordView locale="hub" />} />
<Route exact path="/forgot-password" render={() => <ForgotPasswordView place="hub" />} />

This comment has been minimized.

@Eden12345

Eden12345 Apr 27, 2020
Contributor

Another option is to have a boolean prop called hub. Then, all you would need here is to write <ForgotPasswordView hub />, and for the same component in the extension panel you wouldn't have to pass any prop (which will lead to it being falsey). This will also make the logic in your ForgotPassword.jsx file a little more straightforward. (This is just a suggestion; there is a lot of disagreement online about whether or not this is a good practice, but I prefer it which is why we do it in Insights.)

This comment has been minimized.

@benstrumeyer

benstrumeyer Apr 27, 2020
Author Contributor

I like this idea of boolean props. Changed to hub, however kept the panel variable for better readability

color: #930194;
width: 150px;
}
.button.ghostery-button {

This comment has been minimized.

@Eden12345

Eden12345 Apr 27, 2020
Contributor

Is there no way to use existing Sass from the Hub to take care of this and avoid duplicating markdown? For example, refactoring Sass from individual form components into helper classes in the root Sass file for the Hub, so that you can assign those classes in your ForgotPassword.jsx file when you're in the Hub view?

This comment has been minimized.

@benstrumeyer

benstrumeyer Apr 28, 2020
Author Contributor

Perfect, it looks like we do have the styling for these buttons already. I noticed button widths aren't standardized inside the hub, so we're still going to have to use width here

@benstrumeyer benstrumeyer requested a review from Eden12345 Apr 28, 2020
benstrumeyer and others added 5 commits Apr 28, 2020
@christophertino christophertino added this to the 8.5.0 milestone Apr 28, 2020
@christophertino christophertino merged commit 5bfa9d1 into develop Apr 28, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@christophertino christophertino deleted the GH-2032 branch Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants