-
Notifications
You must be signed in to change notification settings - Fork 2
Leslie/Make style adjustments #60
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
Conversation
12f0484
to
7c9ed9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, but could you help me understand what you did to test? I'm less familiar with the dev workflow for this template. Which pages did you view to see the screenshot? And again for my own understanding, curious what the before / after is, like what does that page look like before you made these changes?
Also, for future changes you can make them in platform-test in the rails-app folder, and if you do so that'll push the changes to a PR environment automatically so the reviewer can take a look themselves too.
Process nits:
- I updated the PR description so that the description isn't all in the blockquote style (i.e. removed the ">" prefix)
- Don't forget to change the PR title to something that follows git commit style guidelines (imperative voice, sentence case) and is understandable to someone with less context ("bring in style changes" doesn't really say what the changes do)
Ahha, my bad. I didn't realize those changes were in I pushed up this code there, too. You can test it here: https://p-167-app-rails-dev-1853701493.us-east-1.elb.amazonaws.com/users/registrations/applicant I did the following tests on that page by manually changing the label and hint text to make it longer for wrapping purposes, and merely toggling the changes on/off in the console to take screenshots. As for walking through the changes: The wrapping of label and legend text. You can note the difference in the Email label (exaggeratedly long in the screenshot) Here's what it looks like before: The I'm not sure how to get you a screenshot of the changes to For the I think that's roughly all the changes? There are some bottom-margin tweaks for lists within |
I also updated the PR title -- I'm lacking much of the context around this myself, so I hope the super literal title is okay |
Looks good to me, feel free to merge. re: commit title, rather than reference another repo, you can look at the commit message from that repo and just use that. For example "Increase form max-width" In this case it's not even that important so I'm also ok with something like "Make style adjustments" |
@LeslieJAnderson for next time, when you merge the PR make sure the commit message follows git commit message guidelines. It should just follow the PR title, so instead of "pulled main and squashed commits" it should be "Make style adjustments" (don't include your name in the PR title or commit message, that's redundant since your name shows up as metadata) |
@include u-measure(5); | ||
padding-top: units(2); | ||
margin-top: 0; | ||
margin-bottom: units(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeslieJAnderson from latest in PFML Starter Kit, we only add the margin-bottom if not displaying an error I think.
Was there a reason to not do that here?
Ticket
Changes
Tweaked styling per instructions; results tested in platform-test-rails
Context for reviewers
Links to initial commits to be copied in the ticket. Was told to bring over all the style stuff except for anything pfml specific.
Testing
Since the text isn't long to demonstrate the proper wrapping, etc, may be a silly screenshot, but demonstrates that the changes don't break anything.