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

feat: updates to the styling for Seeds Buttons and Common App #442

Merged
merged 17 commits into from
Jan 25, 2024
Merged

feat: updates to the styling for Seeds Buttons and Common App #442

merged 17 commits into from
Jan 25, 2024

Conversation

jaredcwhite
Copy link
Collaborator

@jaredcwhite jaredcwhite commented Dec 18, 2023 •
edited
Loading

Issue Overview

This PR addresses #384

NOTE: this would probably supersede the previous release branch: #435

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

This PR provides Doorway styling for Seeds Button, as well as a few fixes so styling of the Common App screens and components match the Figma mockups.

How Can This Be Tested/Reviewed?

Evaluate various screens which use Seeds Buttons, as well the whole Common App flow.

Checklist:

  • My code follows the style guidelines of this project
  • I have added QA notes to the issue with applicable URLs
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have run yarn generate:client and/or created a migration if I made backend changes that require them
  • My commit message(s) is/are polished, and any breaking changes are indicated in the message and are well-described
  • Commits made across packages purposefully have the same commit message/version change, else are separated into different commits

Reviewer Notes:

Steps to review a PR:

  • Read and understand the issue, and ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
  • Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

jaredcwhite and others added 14 commits November 29, 2023 12:28
@jaredcwhite
feat: seeds button uptake (bloom-housing#3677)
bf944b3 
* feat: uptake Grid component from Seeds

* feat: finish Grid component uptake

* feat: update recent changes from main

* fix: mangled syntax

* fix: switch prop to class name

* chore: address linting issues

* fix: typo

* fix: make sure old UIC labels look more modern

* test: use new seeds classes in test selector

* test: resolve issue with grid and Cypress selector

* test: fix bad ids

* feat: first round of uptake of Seeds Button

* fix: grid and form layouts based on feedback

* feat: add local Hero which uses Seeds system

* chore: update StatusItem is with Button/Link

* fix: remove old GridCell, tighten up spacing on Partners aside

* feat: more replacements of Button and Link

* feat: swap Button to use Seeds

* fix: prettier

* test: update tests to use button IDs

* feat: update loading spinners to use new Button feature

* chore: fix linking issues

* test: fix some E2E test cases

* test: fix submission issues with new buttons

* test: add another type submit

* test: finish fixing public E2E tests

* test: button fixes in partners

* test: add another type submit

* test: use more generic button selectors

* fix: preference grid and sign in button types

* fix: missing gap between Edit and Delete

* fix: clean up button color and preferences drawer

* fix: preferences drawer columns

* test: fix partners textarea issue

* fix: other textarea issue

* fix: more Cypress tests

* fix: button scopes

* fix: unit test
@ludtkemorgan
fix: validate radius geocoding preferences (bloom-housing#3718)
a8b13b0 
* fix: validate radius geocoding preferences

* fix: move to more generic

* fix: add test

* fix: one more test

* fix: change to true/false
@KrissDrawing
feat: add address verification for preferences step (bloom-housing#3715)
0f9ead7 
* feat: add collectAddress checkbox with subfields

* test: update preference tests

* fix: add minimum value validation for radius field

* fix: make collect address not required

* fix: expand collect address fields

* fix: change fields order in PreferenceDrawer

* feat: add address holder fields to application

* fix: make added field optional

* fix: display errors properly

* feat: add address holder fields to application summary

* feat: add address verification for preferences step

* feat: adjust padding for application summary

* fix: move address holder name and relationship fields to extraData

* fix: remove redundant backend address holder fields

* fix: use enum for address holder fields

* fix: add alternate address form component

* fix: remove redundant fields from new address form

* fix: verify preferences address when collectAddress true

* fix: block going back on address verification

add string to translation

* fix: use onClick to block address Verification back button
@ludtkemorgan
fix: update dependencies for backend (bloom-housing#3694)
5a8be09
@emilyjablonski
feat: core changes for application style alignment (bloom-housing#3662)
e7af533
@emilyjablonski
test: fix cypress lat long error (bloom-housing#3746)
8751c6e
@emilyjablonski
fix: padding issue on address verify (bloom-housing#3745)
7bf05c1
@emilyjablonski
Merge remote-tracking branch 'upstream/main' into release/2023-12-05
3f41e70
@jaredcwhite
fix: add in missing postcss plugins for public site
1e10ba2
@jaredcwhite
fix: restore missing props on Choose Language page
cd4178d
@jaredcwhite
feat: remove dwy UIC ProgressNav, other styling updates
b65d9c5
@jaredcwhite
feat: card styling and ProgressNav fixes
345c2ce
@jaredcwhite
fix: typo in Readme
90ee836
@jaredcwhite
test: get public site Cypress passing
ce36f39
@jaredcwhite
Copy link
Collaborator Author

OK, still not sure why GH actions aren't running, but Cypress public tests are all green for me locally! I had some problems with the partners tests, but may need help looking at this as I didn't touch anything there previously (so probably a general issue with the Core -> Dwy merge?).

@jaredcwhite jaredcwhite changed the title feat: updates to the styling for Seeds Buttons and Common App (WIP) feat: updates to the styling for Seeds Buttons and Common App Dec 20, 2023
@emilyjablonski
Copy link
Collaborator

@jaredcwhite Is this ready for eyes?

@jaredcwhite
Copy link
Collaborator Author

@emilyjablonski Yes indeed!

@emilyjablonski
Copy link
Collaborator

For this review I was comparing to the Figma listed in the ticket, let me know if any of these discrepancies are expected!
In the designs:

  • The line in the progress navigation component is thicker
  • The main headings like "Let's get started" have a smaller font weight
  • The border under headers like "Choose your language" is blue
  • The text within the main outlined button is a darker blue (but tbh I prefer yours, could be a design question)
  • The back button is a lighter blue
  • The ordered list on the "What to expect" page has a smaller font weight
  • Fields have a darker gray background color behind the input and more of a border radius
  • When a radio or checkbox is selected, the selected color is a darker blue

emilyjablonski
emilyjablonski requested changes Jan 9, 2024
View reviewed changes
Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

Sorry forgot to leave an explicit change!

@jaredcwhite
Copy link
Collaborator Author

@emilyjablonski Thanks for that very detailed feedback! I really had to scrutinize the webpage and the Figma for some of them before I could spot any difference!

So I've just pushed up a set of changes, which should address the points you mentioned except for the following:

  • Regarding the outlined buttons, I also agree it seems weird that the border color and the text color would be different in the Figma. I'd have to do a funky override to make that change, short of updates to Seeds Button.
  • Regarding the back button/link, the color is the "primary" color. I'm not sure why Figma has it set as "primary-light" tbh.

@emilyjablonski
Copy link
Collaborator

emilyjablonski commented Jan 16, 2024 •
edited
Loading

Two new updates for ya:

  • The font weight of the listing title seems higher on this branch than in the designs, which I think may have just been introduced
  • The checkbox color looks great now, but since it's a global override, the selected color of the checkboxes in the filter modal on the listings page (which is a lighter blue) is now the same darker blue - I prefer the version on this PR but we need to check in w design about changing that

And one thing I missed sorry!

  • The color of the progress bar when it is in its complete state is darker in the designs

@jaredcwhite
Copy link
Collaborator Author

@emilyjablonski Thanks for the additional finds…I fixed the progress bar color and the text weight there.

Regarding the checkboxes, I feel it's preferable to use the same style everywhere, unless we specially want a special style for that homepage use case or whatever. Thoughts @slowbot?

@slowbot
Copy link
Collaborator

slowbot commented Jan 19, 2024

@jaredcwhite I agree with having the same checkbox style everywhere as a guideline. Can you provide a reference for where the style is suggested to be an exception?

@slowbot
Copy link
Collaborator

slowbot commented Jan 19, 2024

@jaredcwhite I think I see the checkboxes that are being referenced. I assume they are the ones with blue text. I agree that we can remove that style, as it is color based change only.

@jaredcwhite
Copy link
Collaborator Author

All right, so this is what it looks like now:

image

emilyjablonski
emilyjablonski approved these changes Jan 24, 2024
View reviewed changes
Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

🥳 🥳 🥳

@ludtkemorgan ludtkemorgan added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed labels Jan 25, 2024
@ludtkemorgan
Copy link
Collaborator

@jaredcwhite I haven't done a review of this, but due to the back and forth manner of the review by Emily and Jesse I think this is good to merge with just the one review

@jaredcwhite jaredcwhite merged commit df6c2ae into main Jan 25, 2024
14 of 15 checks passed
@slowbot
Copy link
Collaborator

slowbot commented Feb 2, 2024

@ludtkemorgan @jaredcwhite Where can I QA these items? I don't see any listings on dev or any applications

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilyjablonski emilyjablonski emilyjablonski approved these changes

@ludtkemorgan ludtkemorgan Awaiting requested review from ludtkemorgan

@ColinBuyck ColinBuyck Awaiting requested review from ColinBuyck

Assignees
No one assigned
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jaredcwhite @emilyjablonski @slowbot @ludtkemorgan @KrissDrawing