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-2066, GH-2068, GH-2069, GH-2082, GH-2067 Upgrade Plan panel subscription content #567

Merged
merged 142 commits into from Jun 25, 2020

Conversation

@benstrumeyer
Copy link
Contributor

@benstrumeyer benstrumeyer commented Jun 9, 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?
  • This PR handles the new onboarding view designs in GH-2066 and steals HTML and SASS from ghostery-website
  • Create Yearly/Monthly toggle that changes prices for plus and premium cards
  • UpgradePlanView is responsive. Breakpoints are at 740px, and 960px
  • See Full List of Features scrolls to table below the fold
  • Upgrade Plan sidebar listing is the new default route when the hub is opened
  • Already Protected CTA buttons route to Home, otherwise to the the respective product page for yearly/monthly

Tickets:

benstrumeyer added 30 commits May 7, 2020
… subscriber badge icon
…iptions, and the background handler to return the highest tier subscription
…k, and user.subscription for UI
app/hub/Views/UpgradePlanView/index.js Outdated Show resolved Hide resolved
},
};

export default UpgradePlanView;

This comment has been minimized.

@wlycdgr

wlycdgr Jun 22, 2020
Member

There's a lot of duplication in the return ( ....) code, which makes the code harder to read and modify than it needs to be and increases bug surface area. As the last push on this PR, let's DRY it out. As one example, instead of inlining all the feature matrix rows, we can define (at file scope) a render helper with the following signature:

const featureMatrixRow = (label, inBasic, inPlus, isSparkle) => (
    <tr>
        ...
    </tr>
);

This makes it possible to condense the ~150 lines of inline <tr> elements to:

{featureMatrixRow(t('hub_upgrade_browser_tracker_blocking'), true, true)}
{featureMatrixRow(t('hub_upgrade_browser_ad_blocking'), true, true)}
{featureMatrixRow(t('hub_upgrade_custom_blocking_preferences'), true, true)}
{featureMatrixRow(t('hub_upgrade_extension_themes'), false, true)}
{featureMatrixRow(t('hub_upgrade_historical_extension_stats'), false, true)}
{featureMatrixRow(t('hub_upgrade_application_tracker_blocking'), false, true, true)}
{featureMatrixRow(t('hub_upgrade_application_ad_blocking'), false, true, true)}
{featureMatrixRow('VPN', false, false, true)}
{featureMatrixRow(t('hub_upgrade_no_vpn_logs'), false, false, true)}
{featureMatrixRow(`P2P ${t('support')}`, false, false, true)}
{featureMatrixRow(`IPv6 ${t('hub_upgrade_leak_protection')}`, false, false, true)}
{featureMatrixRow(t('hub_upgrade_physical_servers'), false, false, true)}
{featureMatrixRow(t('hub_upgrade_unlimited_bandwidth'), false, false, true)}

This is one of the main DRYing opportunities, but there are other big ones. At the least, we should do the same for the same batch of <tr> elements for the mobile view. See if you can identify two or three other spots though! We don't need to get this code perfectly DRY, but we should wring it out at least SOME so it doesn't get that mildew smell.

This comment has been minimized.

@benstrumeyer

benstrumeyer Jun 23, 2020
Author Contributor

Yes, we can definitely do this! I wound up DRYing the matrix rows, cards, CTA buttons, and monthly/yearly toggle and it looks much cleaner now. I declared a few of those in component scope instead of file scope so they would have access to props--let me know if there's a better way to do this. As side note, I like how you're intentionally not passing params to those render helper functions so they evaluate to falsy, I used the same trick in a few of the other functions

This comment has been minimized.

@wlycdgr

wlycdgr Jun 23, 2020
Member

Great! This is much cleaner, thank you.
Re: the ones you declared in component scope, with how many props / args we would need to pass to them, I think it's fine to leave them in there. If we get any complaints about performance during QA we know what to try, but I would be very surprised if we did.

@benstrumeyer benstrumeyer requested review from wlycdgr and removed request for Eden12345 Jun 23, 2020
@benstrumeyer benstrumeyer requested a review from wlycdgr Jun 24, 2020
Copy link
Member

@wlycdgr wlycdgr left a comment

lgtm ~

@christophertino christophertino merged commit 441e0b1 into develop Jun 25, 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-2066 branch Jul 9, 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