Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

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

Merged
vdavid merged 2 commits into
mainfrom
contractors/PLG-590
May 13, 2024
Merged

PLG-590 - Add 16px margin between the "$9/month" and CTA#62588
vdavid merged 2 commits into
mainfrom
contractors/PLG-590

Conversation

@gitstart-app

@gitstart-app gitstart-app Bot commented May 10, 2024 •
edited by gitstart-sourcegraph
Loading

Copy link
Copy Markdown
Contributor

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

@cla-bot cla-bot Bot added the cla-signed label May 10, 2024
@gitstart-sourcegraph gitstart-sourcegraph self-assigned this May 10, 2024
vdavid
vdavid approved these changes May 10, 2024
View reviewed changes

@vdavid vdavid left a comment

Copy link
Copy Markdown
Contributor

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

gitstart-sourcegraph commented May 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
@vdavid vdavid deleted the contractors/PLG-590 branch May 13, 2024 10:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chenkc805 chenkc805 Awaiting requested review from chenkc805

1 more reviewer

@vdavid vdavid vdavid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gitstart-sourcegraph gitstart-sourcegraph

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gitstart-sourcegraph @vdavid