CHAL-31 #done

[Ris345]

Checklist

• Include the corresponding Jira issue key and #done in the PR title, like so: "JRA-123 #done Migrate Election Reminders"
• Verify that the code compiles (npm run dev)
• Verify that the project builds (npm run build:local)
• Verify that all tests pass
• Verify that unit tests cover 100% of the code
• Create Storybook stories for visual components N/A
• Verify that any visual components match the Figma N/A
• Test with a screen reader (if applicable) N/A
• Document your code with TSDoc comments
• Format your code with Prettier

Overview

Test Plan

Follow ups

[dvorakjt]

Hi Rishav,

Thanks for the good start on this one. Unfortunately, there are a few changes to request. Besides the smaller issues outlined in my line-by-line comments, the biggest issue is that the buttons continue to overlap each other when placed side-by-side. Please see the attached screenshot. It is possible to fix this, let me know if you have any questions about why this is happening/how to fix it.

Joe

[dvorakjt]

Hello Rishav,

I was able to reproduce the issue we encountered the last time we looked at this. Namely, margins applied to the button class via the className prop were being overwritten by the margin declared in the button's own scss file, as seen here:

When inspecting the styles in Chrome, I observed the following, indicating that the margin applied by the mb_lg was being overwritten, likely because of order of appearance:

To solve this issue, I simply wrapped the styles declared in the button's own stylesheet in an anonymous layer declared with the @layer block rule, producing the following effect:

Because normal styles within layers have lower precedence than unlayered normal styles, the custom styles applied via the className prop now take precedence, enabling us to flexibly adapt this component as needed.

Please see the following MDN articles and make requisite changes (likely just wrapping the button styles in a layer as I did):

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_cascade/Cascade

https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Styling_basics/Cascade_layers

Many thanks!

Joe

[dvorakjt]

This prop should not be necessary and also, it does not conform to the style guide (props should be camelCase)

[dvorakjt]

Approved!

[dvorakjt]

EDIT: I approved this too soon. Your branch is not up-to-date with the upstream repository. You must fetch the latest changes from the upstream repo, merge them locally, resolving merge conflicts in the process, and then push these changes to your fork before this can be approved and merged. It is wise to do this regularly.

If there is a merge conflict in the package-lock.json, simply regenerate it and commit the new file.

There are also formatting issues in the styles file for the Button. I wasn't going to mention these, but since there are other changes to be made, these may as well be addressed. Please be sure to properly indent your code.

In the future, I would like to expect you to keep your code up-to-date before making a PR and to check your own formatting.

[Ris345]

Since this was an ongoing issue, I wanted to ensure we were aligned on the proposed changes before resolving the merge conflict. In the meantime, I've manually corrected the formatting.

I also noticed that our npm run format script seems to be ignoring certain files, which is likely why some formatting inconsistencies are slipping through. It might be worth looking into its configuration to make sure it's covering all the necessary directories.

[dvorakjt]

My mistake, based on our conversation in the last meeting, I was under the assumption that the PR was fully ready to merge. That's a good callout, I think we may need to reconfigure Prettier to ensure that it is formatting SCSS files. Still, in the future, if you notice something like this, feel free to take the initiative to look into it and address it, or to at least manually format the files in the meantime as you did in this case.

[Ris345]

No problem and I can take more initiative going forward in the future. Thanks!