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

feat: show tier modal on account page after logging in #2048

Merged
merged 1 commit into from Oct 18, 2022

Conversation

jsdevel
Copy link
Contributor

@jsdevel jsdevel commented Oct 18, 2022

This shows the user the plan they wanted from the home page after logging in.

@jsdevel jsdevel requested a review from gobengo October 18, 2022 20:11
@@ -2,6 +2,7 @@
* @fileoverview Account Payment Settings
*/

import { parse as queryParse } from 'querystring';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will bundle in a browser polyfill of this thing from nodejs stdlib. wdyt of using the DOM URLSearchParams constructor? https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams

Copy link
Contributor Author

@jsdevel jsdevel Oct 18, 2022

Choose a reason for hiding this comment

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

@gobengo i checked and it looks like we're already bundling it. i can follow up with a tech debt PR if you want to refactor all locations in the codebase to use URL.
querystring

@@ -187,6 +197,9 @@ const PaymentSettingsPage = props => {
onClose={() => {
setIsPaymentPlanModalOpen(false);
setHasAcceptedTerms(false);
if (planQueryParam) {
history.pushState({}, '', window.location.href.replace(/plan=[^&]+&?/, ''));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this inteded to wipe the query param?

If so, I think it's kinda nice to keep it around. That way the user can share the URL to that tier to themselves or others (e.g. via user-agent share functionality, copy url bar and paste elsewhere, etc)

Just curious, what is the motivation of the pushState and, if I'm reading it right, clearing the query param? If it's not strictly needed we might as well not do it.

Copy link
Contributor Author

@jsdevel jsdevel Oct 18, 2022

Choose a reason for hiding this comment

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

so, if the user closes the modal, it's their intention not to see it again. if they refresh the browser after closing the modal and then see the modal again, i figured it would be a little annoying. if they click the browser back button they'll see the modal and query param again with my current implementation. lmk what you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, it's onclose. Nice. Sry for missing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

if they click the browser back button they'll see the modal and query param again with my current implementation.

awesome

Copy link
Contributor

@gobengo gobengo left a comment

Choose a reason for hiding this comment

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

Approach looks great. Approving so as to not block, but probably best to ensure the tsc check passes before merging. re-request review when it's green if you want a pair of eyes on whatever the fix is.

@jsdevel jsdevel force-pushed the feat-show-desired-plan-from-home-page branch from 7eab298 to 13ff0a2 Compare October 18, 2022 20:46
@@ -45,19 +46,36 @@ import GeneralPageData from '../../content/pages/general.json';
* @property {PaymentMethodCard} card
*/

function removePlanQueryParam() {
Copy link
Contributor

Choose a reason for hiding this comment

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

love how the function name is self-documenting 🙏

This shows the user the plan they wanted from the home page after logging in.
@jsdevel jsdevel force-pushed the feat-show-desired-plan-from-home-page branch from 13ff0a2 to 072a5f5 Compare October 18, 2022 20:57
@jsdevel
Copy link
Contributor Author

jsdevel commented Oct 18, 2022

Approach looks great. Approving so as to not block, but probably best to ensure the tsc check passes before merging. re-request review when it's green if you want a pair of eyes on whatever the fix is.

yea, pushed up some changes to make the build pass. fingers crossed...

@github-actions
Copy link
Contributor

@jsdevel jsdevel merged commit 1204561 into main Oct 18, 2022
@jsdevel jsdevel deleted the feat-show-desired-plan-from-home-page branch October 18, 2022 21:10
gobengo pushed a commit that referenced this pull request Oct 19, 2022
🤖 I have created a release *beep* *boop*
---


##
[2.29.0](website-v2.28.0...website-v2.29.0)
(2022-10-18)


### Features

* enterprise let's chat form
([#2034](#2034))
([ec2bb57](ec2bb57))
* show tier modal on account page after logging in
([#2048](#2048))
([1204561](1204561))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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

2 participants