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.

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

Fix various minor issues #11960

Merged
merged 12 commits into from
Mar 9, 2024
Merged

Fix various minor issues #11960

merged 12 commits into from
Mar 9, 2024

Conversation

mmmavis
Copy link
Collaborator

@mmmavis mmmavis commented Feb 29, 2024

Related PRs/issues:

Review app login: https://foundation-s-fix-minor--meywng.herokuapp.com/cms/ (credential: admin2/admin2password)

#9666 Enlarged nav dropdown's vertical gap to 16px

Commit: 2e9c599
Test page: https://foundation-s-fix-minor--meywng.mofostaging.net/en/who-we-are/

#10857 Adjusted PNI category dropdown button font size

Commit: b8b8de3
Test page: https://foundation-s-fix-minor--meywng.mofostaging.net/en/privacynotincluded/

#9941 Capitalized all instances of lowercase *PNI

Commit: 17f1f19
Test page: https://foundation-s-fix-minor--meywng.mofostaging.net/en/privacynotincluded/
Note for code reviewer: Commit includes a migration file due to updating a verbose_name but I'm debating if this change is really worth having a migration file for or we just leave this particular instance of lowercase *PNI as is.

#10121 Made spacer available on ArticlePage

Commits: ddccad1 and fea35ba
Test page: https://foundation-s-fix-minor--meywng.herokuapp.com/cms/pages/127/edit/ (credential: admin2/admin2password)

@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng February 29, 2024 23:50 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 1, 2024 00:02 Inactive
@mmmavis mmmavis changed the title [WIP] Fix minor styling issues [WIP] Fix minor styling and copy issues Mar 1, 2024
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 1, 2024 01:02 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 1, 2024 18:51 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 1, 2024 19:58 Inactive
@mmmavis mmmavis force-pushed the fix-minor-styling-issues branch from 6a3c075 to fea35ba Compare March 1, 2024 19:59
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 1, 2024 19:59 Inactive
@mmmavis mmmavis force-pushed the fix-minor-styling-issues branch from fea35ba to bef46d7 Compare March 1, 2024 20:14
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 1, 2024 20:14 Inactive
@mmmavis mmmavis changed the title [WIP] Fix minor styling and copy issues [WIP] Fix vairious minor issues Mar 1, 2024
@mmmavis mmmavis changed the title [WIP] Fix vairious minor issues [WIP] Fix various minor issues Mar 1, 2024
@mmmavis mmmavis marked this pull request as ready for review March 1, 2024 21:48
@mmmavis mmmavis changed the title [WIP] Fix various minor issues Fix various minor issues Mar 1, 2024
Copy link
Contributor

@jhonatan-lopes jhonatan-lopes left a comment

Choose a reason for hiding this comment

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

Hey @mmmavis, I just have a minor observation

Copy link
Collaborator

@nancyt1 nancyt1 left a comment

Choose a reason for hiding this comment

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

For #9941, the ding icon on product pages (#4) is still showing up lowercase. We could also pull this out into another ticket if it's too much work since it's relatively minor. Other than that, everything else looks great!

@mmmavis
Copy link
Collaborator Author

mmmavis commented Mar 5, 2024

For #9941, the ding icon on product pages (#4) is still showing up lowercase. We could also pull this out into another ticket if it's too much work since it's relatively minor. Other than that, everything else looks great!

@nancyt1 I just took a look but couldn't reproduce the issue you found. Do you mind checking again?

image

@nancyt1
Copy link
Collaborator

nancyt1 commented Mar 5, 2024

@mmmavis
It's this icon that still uses lowercase.

SCR-20240305-ouqj

@mmmavis
Copy link
Collaborator Author

mmmavis commented Mar 5, 2024

@nancyt1 Ah I see. Thanks for pointing that out. In that case could I ask you to help update this image with upper case text? If it takes too much effort, I can file a new ticket for us to tackle later

@nancyt1
Copy link
Collaborator

nancyt1 commented Mar 5, 2024

@mmmavis Here's the folder of the SVGs! It also has localized versions.

@mmmavis
Copy link
Collaborator Author

mmmavis commented Mar 5, 2024

@nancyt1 Thanks! And sorry I didn't realize you included the link in the initial ticket already. Time to get some coffee Mavis ☕

@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 5, 2024 23:33 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 7, 2024 18:56 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 7, 2024 20:08 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 7, 2024 22:13 Inactive
@mmmavis
Copy link
Collaborator Author

mmmavis commented Mar 7, 2024

So that we can have these old tickets addressed and closed sooner, I've replaced the current ding images with the new ones. In the future, we should move towards excluding text from images. This will require some dev effort but should be pretty strightforward to implement.

@nancyt1 Please verify the changes here: https://foundation-s-fix-minor--meywng.herokuapp.com/en/privacynotincluded/gas-finish-hit-speak-eye-store/ Thanks!

@mmmavis mmmavis requested a review from nancyt1 March 7, 2024 22:18
@nancyt1
Copy link
Collaborator

nancyt1 commented Mar 7, 2024

@mmmavis hm, they still show up as lowercase for me. Can you screenshot what you see?

@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 7, 2024 23:06 Inactive
@mmmavis
Copy link
Collaborator Author

mmmavis commented Mar 7, 2024

@nancyt1 sorry, I updated everything except the English version. This should be fixed now 😅

Copy link
Collaborator

@nancyt1 nancyt1 left a comment

Choose a reason for hiding this comment

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

All looks good! (:

@mmmavis mmmavis temporarily deployed to foundation-s-fix-minor--meywng March 9, 2024 00:50 Inactive
@mmmavis mmmavis merged commit 18baf92 into main Mar 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants