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.

Sign up for GitHub

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

Jump to bottom

tomc/design polish #24

Merged
merged 15 commits into from
Oct 2, 2023
Merged

tomc/design polish #24

merged 15 commits into from
Oct 2, 2023

Conversation

RandomEtc
Copy link
Member

@RandomEtc RandomEtc commented Sep 30, 2023 •
edited
Loading

Changes

  • set state card-content padding to 1.5rem except on small screens
  • override border variables for state embeds
  • use sentence case for labels
  • state calculator form labels are uppercase
  • tooltip text is not supposed to transform
  • persnickety apostrophes
  • state calc button is purple, no icon
  • don't focus the ra-currency-input, only its child input
  • clean up select and input styles for state calc
  • css variables for inputs
  • adjust input and select margins to match state design

Should close these issues

Test plan

Screenshots

Before:
Screenshot 2023-09-30 at 8 56 59 PM

After:
Screenshot 2023-09-30 at 8 57 02 PM

@vercel
Copy link

vercel bot commented Sep 30, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
embed-rewiringamerica-org ✅ Ready (Inspect) Visit Preview Oct 2, 2023 7:41pm

@RandomEtc
Copy link
Member Author

RandomEtc commented Oct 1, 2023 •
edited
Loading

Noting for later:

  • some of the style changes don’t look quite right on mobile - the select text color is blue
  • for some reason the text in the utility selector is still small (I think that means the select needs un-nesting from its label)

oyamauchi
oyamauchi approved these changes Oct 2, 2023
View reviewed changes
Copy link
Member

@oyamauchi oyamauchi left a comment

Choose a reason for hiding this comment

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

Unfortunately I think this is going to cause a bunch of rebase work on #21 again... sorry @ayangster

Approving with minor comments... especially the one about smart quotes? I assume you're using some tool to do those; is there a way to have it not do smart quotes in code comments?

.vscode/settings.json Outdated Show resolved Hide resolved
src/api/fetch.ts Outdated Show resolved Hide resolved
src/calculator-form.ts
${select({
id: 'owner_status',
required: true,
options: OWNER_STATUS_OPTIONS,
Copy link
Member

Choose a reason for hiding this comment

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

Given the label change, maybe change the option text to match? Rent and Own?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought to leave it as-is because the API field is owner_status - rather than overfit to the current preferred UI label which will be translated in future. But happy to switch it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait, I see. I'm going for parity with the current https://www.rewiringamerica.org/app/ira-calculator calculator here. I'll make a Design ticket to follow up. There was some back and forth on this with our Marketing folks so I'll include them. https://app.asana.com/0/1205057814651000/1205635758958629/f

@RandomEtc
Copy link
Member Author

Unfortunately I think this is going to cause a bunch of rebase work on #21 again... sorry @ayangster

Let's land #21 first, I can rebase this one. It'll be easier for me to carry context over, I restructured a couple of things.

@RandomEtc
Copy link
Member Author

I removed the apostrophe edits (#30) and the vscode config (#29).

@RandomEtc
Copy link
Member Author

Alright, rebased with #21 included. Should be good to go!

@RandomEtc RandomEtc merged commit 7c31c7a into main Oct 2, 2023
1 check passed
@RandomEtc RandomEtc deleted the tomc/design-polish branch October 2, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oyamauchi oyamauchi oyamauchi approved these changes

@ayangster ayangster Awaiting requested review from ayangster

@mariabajzek mariabajzek Awaiting requested review from mariabajzek

Assignees

@oyamauchi oyamauchi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RandomEtc @oyamauchi