Grant Details Page - move grants.gov, copy, print buttons #3184
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
javascript
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @amyewang, Action: |
amyewang
force-pushed
the
amyewang/grant-details-move-copy-print-grantsdotgov
branch
2 times, most recently
from
300f447 to
338d4c9
Compare
jeffsmohan
suggested changes
Jun 18, 2024
There was a problem hiding this comment.
This is looking really good! Just a few minor cleanups in inline comments, plus:
- Might be worth checking with the designers, but to my eye I think the designs had the button going down a size smaller, plus the margin between the button/links and the rest of the content being a bit smaller as well
| role="button" | ||
| :variant="copyUrlSuccessTimeout === null ? 'outline-primary' : 'outline-success'" | ||
| data-dd-action-name="copy btn" | ||
| style="white-space: nowrap; " |
There was a problem hiding this comment.
In general, we should avoid inline styles where possible, since they're harder to maintain, keep consistent, override, etc. Instead, we want to favor existing utility classes where available, or styles defined in a style section of the component otherwise.
In this case, you should be able to use the text-nowrap utility class provided by Bootstrap. (Docs here)
There was a problem hiding this comment.
thanks! getting used to Bootstrap
| variant="primary" | ||
| block | ||
| class="mr-5" | ||
| style="width: fit-content; white-space: nowrap; " |
There was a problem hiding this comment.
A cleaner way to achieve the new layout, using existing bootstrap helpers:
- Remove the
blockoption (instead of thewidth: fit-contentstyle) - Add the
text-nowraputility class (instead of thewhite-space: nowrapstyle) - You can remove the
mr-5class now that we don't need a right margin on the button
There was a problem hiding this comment.
I left a margin to the right of the button to add some negative space between "view" and "copy" in the case of the minimum viewport width - this wasn't really specified in the designs though. Let me know what you think
There was a problem hiding this comment.
Good call! Sorry, I just assumed it had been carried over without intention from the previous arrangement, my bad.
Longer-term, I know the team is looking at supporting mobile designs on smaller screen sizes, so we might be able to address this sort of thing even more robustly...
| role="button" | ||
| variant="outline-primary" | ||
| data-dd-action-name="print btn" | ||
| style="white-space: nowrap; " |
| <span v-if="copyUrlSuccessTimeout === null">Copy Link</span> | ||
| <span v-else>Link Copied</span> | ||
| </a> | ||
| <div class="col-1 border-right border-dark p-2"/> |
There was a problem hiding this comment.
Seeing some linting errors here. Ideally you shouldn't have to think about style stuff like this, and you can get your editor set up to auto-format on save. A bit of documentation on getting set up is here — https://github.com/usdigitalresponse/usdr-gost/blob/main/docs/development.md#linting — but lmk if you have any trouble getting that working!
amyewang
force-pushed
the
amyewang/grant-details-move-copy-print-grantsdotgov
branch
from
387b2de to
67ac409
Compare
amyewang
force-pushed
the
amyewang/grant-details-move-copy-print-grantsdotgov
branch
from
67ac409 to
9ee390d
Compare
|
Thanks for the review @jeffsmohan ! I fixed the inline styling, updated the overall button & link sizing (thanks for that callout), and updated my linting. Left 1 small comment about the button margin. |
jeffsmohan
suggested changes
Jun 21, 2024
There was a problem hiding this comment.
This looks basically ready to go!
Only thing is let's avoid changing the workspace settings, at least in this PR. I'd love to discuss what linting issues you were running into that this solved for you, and then maybe we can turn that into a separate PR to get linting set up more robustly for folks!
usdr-gost.code-workspace
Outdated
| @@ -26,6 +26,6 @@ | |||
| // competing user settings and workplace settings. We use VSCode's "codeActionsOnSave" | |||
| // to manage linting/fixing, but users can have other settings via "formatOnSave". | |||
| // If both are on, the editor can "fix" things that are immediately flagged as lint failures. | |||
| "editor.formatOnSave": false, | |||
| "editor.formatOnSave": true, | |||
There was a problem hiding this comment.
I don't think we want to check this change in. Were you not able to get automatic linting on save working without this change?
I'd love to look into that a bit more with you... the codeActionsOnSave is supposed to supersede the formatOnSave setup in VSCode I believe, but the setup is finicky, and I didn't have many users to test out the settings with when I was changing them last...
There was a problem hiding this comment.
whoops sorry this was careless, I was playing with settings and accidentally committed this. my bad.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket #3153
Description
Update the Grant Details page main actions buttons to match expected state in the Figma designs
Moved to the right column, updated text and icons, updated "copy" and "print" to be links.
Screenshots / Demo Video
Testing
Tested various window widths in local dev env
Automated and Unit Tests
Manual tests for Reviewer
Checklist