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

PLG-590 - Add 16px margin between the "$9/month" and CTA #62588

Merged
merged 2 commits into from May 13, 2024

Conversation

gitstart-app[bot]
Copy link
Contributor

@gitstart-app gitstart-app bot commented May 10, 2024

Description

Add a 16px margin between "$9/month" and CTA on the cody/subscription page particularly on the Pro card

Changes made

Added a style class called "pro-margin-top" to the CodySubscriptionPage.module.scss file and then added the CTA section

Refs

GitStart Ticket
PLG Issue

Success Criteria

On inspection, margin between the sections should be 16px

pro-price-1 pro-price-2

Checklist

  • I have tested the changes locally
  • My code follows the project's coding style
  • I have updated the documentation accordingly
  • I have added unit tests for my changes
  • All tests are passing
  • I have squashed any irrelevant commits
  • I have rebased the PR to the latest main/development branch
  • I have resolved any merge conflicts
  • My changes do not introduce any new warnings or errors
  • I have reviewed my own code for any potential issues

Demo Video:

https://www.loom.com/share/1661427edd4b4ceb92ad4dd3bde31a79?sid=b8fb38a9-b5a8-4674-a207-440d2943ced6

Test Plan

  1. Run "sg start dotcom"
  2. Navigate to "https://sourcegraph.test:3443/cody/subscription"
  3. Check Pro card using inspection
  4. To check second case, head to client/web/src/cody/subscription/CodySubscriptionPage and on line 206 refactor isProUser to `!isProUser

Copy link
Contributor

@vdavid vdavid left a comment

Choose a reason for hiding this comment

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

The solution looks right but the Loom talks about and displays top margin while the change involves setting the bottom margin (of which, the latter is the idiomatic solution). This feels contradictory (maybe the Loom was done with a different change on the code?), but approving the PR because the change looks right.

@gitstart-sourcegraph
Copy link
Collaborator

gitstart-sourcegraph commented May 13, 2024

The solution looks right but the Loom talks about and displays top margin while the change involves setting the bottom margin (of which, the latter is the idiomatic solution). This feels contradictory (maybe the Loom was done with a different change on the code?), but approving the PR because the change looks right.

Hi @vdavid I forgot to remove the loom video after the app failed to run locally. I will ensure to update everything else going forward. Here is loom video
Thank you

@vdavid vdavid merged commit ab575f8 into main May 13, 2024
9 of 14 checks passed
@vdavid vdavid deleted the contractors/PLG-590 branch May 13, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants