-
Notifications
You must be signed in to change notification settings - Fork 143
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
Onboarding spreadsheet bugs #38 (GH-2259), #44 (GH-2306), and #45 (GH-2305) #675
Conversation
benstrumeyer
commented
Feb 2, 2021
•
edited
Loading
edited
- 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?
- Bug 38, Make Glow logo larger in search selection modal
- Bug 44, Center align ghostery glow icon with the box
- Bug 45, Select plus plan and expand selection if user is basic and has chosen ghostery search
…n modals to better match Zeplins
…er. Show expanded view if user picked Glow to match expected UX. Add TODOs so we remember to update AccountReducer to help distinguish between no user present and user not fetched yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good ~ Couple more things:
- I made a render helper for the checkmark items in the option cards and started refactoring the code to use it - if you could finish that up that would be great
- Let's factor the
isBasic
,isPlus
, andisPremium
checks out to helpers. First cos they're duplicated inrender
andsetDefaultPlan
, but also we reduce chance of error nicely with the logic for these checks being a bit convoluted. - Speaking of
setDefaultPlan
- as far as I can tell from a combination of memory and double-checking just now,setTimeout
is non-blocking, so with the code as it was,render
would run beforesetDefaultPlan
anyway (you can see this in action by settingreadyToRender
totrue
from the get-go and throwing in a couple consologs). To make sure we actually don't render until aftersetDefaultPlan
has run after the timeout period, I added thisreadyToRender
state variable that defaults tofalse
and short-circuitsrender
until it's set totrue
at the end ofsetDefaultPlan
. Nothing to do here at the moment really, just wanted to point this out (ultimately though it'd be good to update theAccountReducer
so that we have a way of distinguishing between 'no user' and 'user data not fetched yet'). - I took the liberty of making some additional adjustments to search selection modal logo sizes to better match the Zeplins, since that was on me to begin with. Let me know if you see that I screwed anything up and I'll fix.
Thank you!
…r to reduce duplication
Done |
…e duplication and bug surface
Done |
… to avoid using setTimeout to setState in constructor
Reworked to call |
Double checked that this looks ok, but a fresh pair of eyes would be appreciated |
…ing renders as expected when user is Plus or Premium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…-2305) (#675) * Expand toggle and select plus plan if user is basic and selects ghostery search * Make ghostery glow icon bigger in select default search modal * Move ghostery logo on search option selection container a little to the left * Remove expansion by default * Update logo sizes on choose default search view * Adjust sizing for Yahoo, Startpage, and Bing logos in search selection modals to better match Zeplins * Increase chance user object is available before choose plan view render. Show expanded view if user picked Glow to match expected UX. Add TODOs so we remember to update AccountReducer to help distinguish between no user present and user not fetched yet * Factor out option card checkmark item rendering to a helper to reduce duplication. * Update test snapshots * Refactor choose plan view plan card feature copy rendering to a helper to reduce duplication * Factor user status checks in select plan view out to helpers to reduce duplication and bug surface * Move setting of default plan in plan select view to componentDidMount to avoid using setTimeout to setState in constructor * Revised setDefaultPlan() logic in ChoosePlanView to make sure everything renders as expected when user is Plus or Premium Co-authored-by: wlycdgr <ilya.zarembsky@gmail.com>
…-2305) (#675) * Expand toggle and select plus plan if user is basic and selects ghostery search * Make ghostery glow icon bigger in select default search modal * Move ghostery logo on search option selection container a little to the left * Remove expansion by default * Update logo sizes on choose default search view * Adjust sizing for Yahoo, Startpage, and Bing logos in search selection modals to better match Zeplins * Increase chance user object is available before choose plan view render. Show expanded view if user picked Glow to match expected UX. Add TODOs so we remember to update AccountReducer to help distinguish between no user present and user not fetched yet * Factor out option card checkmark item rendering to a helper to reduce duplication. * Update test snapshots * Refactor choose plan view plan card feature copy rendering to a helper to reduce duplication * Factor user status checks in select plan view out to helpers to reduce duplication and bug surface * Move setting of default plan in plan select view to componentDidMount to avoid using setTimeout to setState in constructor * Revised setDefaultPlan() logic in ChoosePlanView to make sure everything renders as expected when user is Plus or Premium Co-authored-by: wlycdgr <ilya.zarembsky@gmail.com>