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

Change plan widget #1649

Merged
merged 25 commits into from
Nov 4, 2021
Merged

Change plan widget #1649

merged 25 commits into from
Nov 4, 2021

Conversation

RyRy79261
Copy link
Contributor

closes #1455

@render
Copy link

render bot commented Oct 21, 2021

@render
Copy link

render bot commented Oct 21, 2021

@render
Copy link

render bot commented Oct 21, 2021

@FSM1 FSM1 changed the base branch from dev to epic/files-billing October 22, 2021 15:23
@lgtm-com
Copy link

lgtm-com bot commented Oct 22, 2021

This pull request introduces 3 alerts when merging 4cb1bd9 into c6e935b - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@render
Copy link

render bot commented Oct 25, 2021

@lgtm-com
Copy link

lgtm-com bot commented Oct 25, 2021

This pull request introduces 3 alerts when merging 150aa31 into c6e935b - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@RyRy79261 RyRy79261 marked this pull request as ready for review October 25, 2021 10:48
@RyRy79261 RyRy79261 requested review from tanmoyAtb, FSM1 and Tbaut October 25, 2021 10:48
FSM1 added 2 commits October 25, 2021 17:01
* Fix build for node 17

* lingui extract

Co-authored-by: GitHub Actions <actions@github.com>
(cherry picked from commit 226254c)
Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Overall looks good,
Although the API keeps throwing 500 at me, couldn't test everything.

The loaders looks too big for my liking. could make them smaller.
The mobile view also looks cramped. Not sure if we have designs for them.

Left a few comments.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

A couple comments on the UI, will dig in the code later.
image

  • "current" is hard to read because of the contrast
  • "not sure what to pick" font should prob. be bigger
  • "Free" disappears when we toggle the annual plan
  • button toggle doesn't tell whether or not it's activated. Should it be green then?
  • when you select another plan than yours, you can't select the free plan any more, it's disabled. I'd let users actually select it (and disable the apply button)

Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

One flow that has not yet been catered for is if a user attempts to upgrade their subscription but does not have a card added to their account yet.

@RyRy79261
Copy link
Contributor Author

@Tbaut

  • The current tag was improv, so not sure what would be preferred here
  • Adjusted the font variant
  • There isn't any price data for Free's annual variant so theres nothing to interpret or display
  • Potentially, I just used the design and single colors since it works in dark mode
  • Sweet removed the click blocking, though now the user wont know why they cant select a plan until they choose it

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I went through the code, the "Free" flashing when we change the yearly/monthly toggle isn't a big deal I guess. Regarding the color, the "Current" plan is super hard to read in light mode with black on blue. We should have white on blue

RyRy79261 and others added 4 commits October 27, 2021 12:38
Co-authored-by: Tanmoy Basak Anjan <tanmoy3399@gmail.com>
…ngeProductViews/SelectPlan.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…ngeProductModal.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
RyRy79261 and others added 6 commits October 27, 2021 13:23
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…ngeProductModal.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…ngeProductModal.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@RyRy79261 RyRy79261 requested review from Tbaut, FSM1 and tanmoyAtb October 28, 2021 11:51
@Tbaut
Copy link
Collaborator

Tbaut commented Oct 29, 2021

as per my previous comment #1649 (comment)

type casting will make the app crash if the selectPlan is undefined. we should rather add an early return like if(!selectedPlan) return

Type casting should be last resort, I'm pretty sure we can be safer than this.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

see previous comment

@RyRy79261
Copy link
Contributor Author

@Tbaut The button can only be executed if the value is not undefined, I've added a redundant check to remove the casting

@RyRy79261 RyRy79261 requested a review from Tbaut November 4, 2021 06:31
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Looking great!

@Tbaut Tbaut enabled auto-merge (squash) November 4, 2021 11:17
@Tbaut Tbaut merged commit 5bb4490 into epic/files-billing Nov 4, 2021
@Tbaut Tbaut deleted the feat/change-plan-1455 branch November 4, 2021 12:13
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.

Select plan modal screen
4 participants