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

Updated scss to only target h2 on privacy policy page #4945

Merged

Conversation

angelenelm
Copy link
Member

@angelenelm angelenelm commented Jul 13, 2023

Fixes #4864

What changes did you make?

  • Update h2 selector on lines 52-54 (within @media #{$bp-desktop-up}) to target h2s in .content-section--privacy-policy class, which corresponds to h2s in the Privacy Policy card

Why did you make the changes (we will use this info to test)?

Screenshots

Visuals before changes are applied

Join Us page:
Screen Shot 2023-07-12 at 8 17 32 PM

Privacy Policy page:
Screen Shot 2023-07-12 at 8 17 54 PM

Visuals after changes are applied

Join Us page (headers here are no longer being affected):
Screen Shot 2023-07-12 at 8 17 36 PM

Privacy Policy page (headers here are still affected):
Screen Shot 2023-07-12 at 8 17 56 PM

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b angelenelm-privacy-policy-page-change-h2-4864 gh-pages
git pull https://github.com/angelenelm/website.git privacy-policy-page-change-h2-4864

@github-actions github-actions bot added role: front end Tasks for front end developers Complexity: Medium P-Feature: Project Info and Page A project's detail page (e.g. https://www.hackforla.org/projects/100-automations) time sensitive Needs to be worked on by a particular timeframe P-Feature: Join Page https://www.hackforla.org/join P-Feature: Privacy Policy https://www.hackforla.org/privacy-policy size: 1pt Can be done in 4-6 hours labels Jul 13, 2023
@roslynwythe roslynwythe requested a review from kiwookim July 16, 2023 17:12
@kiwookim
Copy link
Member

ETA: July 17 EOD
Availability: 9-5pm PST

@steven-positive-tran
Copy link
Member

ETA: July 17 EOD
Availability: 12-6pm PST

@steven-positive-tran steven-positive-tran self-requested a review July 17, 2023 18:21
Copy link
Member

@steven-positive-tran steven-positive-tran left a comment

Choose a reason for hiding this comment

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

Branches look good, code changes seem in line with the issue that needed to be addressed.
I see that 311 page and the join us page the margin at the top for each card is reduced which is the expected outcome.

Good job and thank you for taking up this issue for hackforla.

kiwookim
kiwookim previously approved these changes Jul 18, 2023
Copy link
Member

@kiwookim kiwookim left a comment

Choose a reason for hiding this comment

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

Thanks for taking this issue @angelenelm.
Branch name looks good, and I checked using docker to confirm that there aren't any breaking changes as a result of applying a class to affect only the h2 elements in privacy policy page.

@roslynwythe @t-will-gillis -- tagging you guys as Roslyn suggested during Sunday meeting regarding one of the linter not passing.

@t-will-gillis t-will-gillis self-requested a review July 18, 2023 16:49
@ronaldpaek ronaldpaek assigned ronaldpaek and unassigned ronaldpaek Jul 19, 2023
@ronaldpaek ronaldpaek self-requested a review July 19, 2023 14:40
ronaldpaek
ronaldpaek previously approved these changes Jul 19, 2023
Copy link
Member

@ronaldpaek ronaldpaek left a comment

Choose a reason for hiding this comment

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

Thank you for taking up this issue, everything works perfectly on my end no issue with the linter.

@ronaldpaek
Copy link
Member

When I run it on my own repo the test case passes, but the current test keeps failing, how weird

@ronaldpaek
Copy link
Member

I wonder if you can refactor to

.content-section--privacy-policy {
	padding-top: 34px;
    padding-bottom: 34px;
    h2 {
      margin-top: 48px;
    }
}

I'm also not the best in SCSS, haven't really used it much.

@ronaldpaek
Copy link
Member

@t-will-gillis It pass locally on my side, but still seems like it is failing here, you can figure it out and then merge.

@ronaldpaek
Copy link
Member

ronaldpaek commented Jul 19, 2023

@t-will-gillis if there is still an issue I was thinking of trying to add this rule set in the .stylelintrc.json file

I mean, try to add this to the .stylelintrc.json file. Obviously, add the rest of the other rules already on the stylelintrc.json file as well, but I don't know if this will work cause says we need to npm install dev dependency, but it might work without it.

{
"plugins": [
"stylelint-scss"
],
"rules": {
"scss/at-rule-no-unknown": true,
// Additional rules
}
}

@angelenelm
Copy link
Member Author

After some research I saw that the error could be thrown due to the comment syntax starting on line 60. Stylelint doesn't support SCSS but can with this config. Let me know what the best course of action should be :)

@angelenelm
Copy link
Member Author

I saw the check is failing at line 30, which isn't a section I touched so I'm not sure what's going on

@ronaldpaek
Copy link
Member

this config

try it, try to modify the .stylelintrc.json file to extend the package you are referring to see if it works without npm install, I'm still kind of confused about npm install and package.json for this whole project cause there isn't one in the root.

Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

hi @angelenelm Thanks for your patience! Your issue looks great as the previous reviewers have indicated.

The 'fail' that is happening with the Lint SCSS is likely caused by some conflicts that occurred when the we updated github/super-linter to v5.0.0 per the Dependabot. Some other PRs/ issues are being flagged with the same 'fail' message, so this is not unique to yours.

Would you please revert the changes to .stylelintrc.json? After that we can merge your PR. We will open a new issue to address the Lint SCSS.

Thank you!

@angelenelm
Copy link
Member Author

@t-will-gillis Thanks so much for the explanation! The changes to that file have been removed.

@t-will-gillis t-will-gillis merged commit a708b50 into hackforla:gh-pages Jul 21, 2023
@angelenelm angelenelm deleted the privacy-policy-page-change-h2-4864 branch July 21, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Medium P-Feature: Join Page https://www.hackforla.org/join P-Feature: Privacy Policy https://www.hackforla.org/privacy-policy P-Feature: Project Info and Page A project's detail page (e.g. https://www.hackforla.org/projects/100-automations) role: front end Tasks for front end developers size: 1pt Can be done in 4-6 hours time sensitive Needs to be worked on by a particular timeframe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change h2 margin on Privacy Policy Page
5 participants