Applied Title classes to heading elements on donate page

[pattshreds]

Fixes #2084

What changes did you make and why did you make them ?

• Applied Title classes to heading elements so that they don't relay on Hx styling rules.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

Visuals after changes are applied

[github-actions]

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 pattshreds-apply-design-system-classes-donate-2084 gh-pages
git pull https://github.com/pattshreds/website.git apply-design-system-classes-donate-2084

[JessicaLucindaCheng]

@pattshreds Good job noticing the header didn't align with the paragraph. I have some inline request for changes. Also, please do the following:

1. Please add your pull request to the project board.

2. I noticed your changes impact more than just the portion in your screenshot. Please screenshot the whole page by using the GoFullPage Chrome extension: https://chrome.google.com/webstore/detail/gofullpage-full-page-scre/fdpohaocaechififmbbbbbknoalclacl?hl=en

3. You don't need to define the title classes in _sass/components/_donate.scss because they already exist in our codebase. So, please remove the title classes you added in that file.

4. When applying the title classes to the HTML, the class name should be lowercase. For example, <h1 class="title1">.

[JessicaLucindaCheng]

@pattshreds I wasn't sure if you were ready for me to review your pr again but I went ahead and did it anyway. (Sorry if it wasn't ready and you were still working on it.) In addition to my in-line comments for changes, please do the following:

• Since changes to the _guide-pages/2FA.html is not part of your pr, to try to fix that, please update your forked gh-pages so that it is in sync with the hackforla/website gh-pages and then update your topic branch. Here are instructions for how to update your forked gh-pages and the how to update your topic branch.

• When you are ready for me to re-review your pull request, please click the following in the Reviewers section on the right:

[JessicaLucindaCheng]

@pattshreds Good job changing the headers tags to the correct hierarchy and merging updates from gh-pages.

• When I compared the Donate page with the changes you made to the Donate page on the live website, the headers look different in terms of font size. The Donate page headers should still look the same as the ones on the live website. Since the font-size for the headers now should only depend on the title classes, the font-sizes for any headers should be removed from _sass/components/_donate.scss. (Note: In that SCSS file, there are font-sizes for other elements besides headers.)
• Check your changes locally using Docker and make sure everything still looks the same as Donate page on the live website
• When you are ready for me to re-review your pull request, please click the following in the Reviewers section on the right:
  

[SAUMILDHANKAR]

@pattshreds Hope you are doing well. Just wanted to share one of the decisions made during our team meeting. We are now requesting all website team members to attend at least one meeting in a week (Tuesday, Thursday or Sunday) as we strive to replicate a professional working environment within our team amongst other things. Therefore, request you to let us know which of the weekly meetings would be suitable for you based on availability. We have decided to make an exception for this requirement only on a case-by-case basis. I will leave a message for you on slack as well. Thank you.

[Carlos-A-P]

@pattshreds Great job adding the classes and changing the header tags to have a proper header structure. As Jessica mentioned, all you need to do is remove any header styling in the _donate.scss file to resolve the issue

[SAUMILDHANKAR]

Heard back from Patrick on slack. Would be updating progress this week.

[pattshreds]

Progress: Title classes are assigned in HTML.
Blockers: The title 4 element and the first title7 element don't seem to be the correct sizes. Need to figure out where to define their sizes.
Availability: This week 6pm - 8pm
ETA: Thursday 3/24/22

[JessicaLucindaCheng]

• Availability: 3 hours
• Review ETA: Sat, April 30, 2022

Started reviewing but didn't finish. Will finish Saturday.

[JessicaLucindaCheng]

@pattshreds Thanks for updating your topic branch with the lastest updates. In addition to my in-line request for changes, please do the following:

• Please update your first post as follows:

  • Update what you did by removing the following (since you are no longer doing the following):

    • "Repositioned the first H1 element on the page to align with the P below it according to the donate page Figma."
    • "Defined the used Title classes in the _donate.scss file."

  • Update the screenshots for the visuals for the before and after. Since you are changing the headers for the whole Donate page, you may want to use GoFullPage - Full Page Screen Capture Chrome Extension in order to screenshot the whole page in desktop and mobile views.

• When you are ready for me to re-review your pull request, please click the following in the Reviewers section on the top right:
  

[Sparky-code]

Avail: 1-2hrs
ETA: 5/18-19 EOD

[trishajjohnson]

Availability: 2 hrs
ETA: EOD 5/19/22

[trishajjohnson]

@SAUMILDHANKAR needing clarification, it appears this issue dates back quite a ways. I'm assuming you just want me to review the most recent changes dating back to the last PR review done by Katherine on April 5, or am I mistaken?

[Sparky-code]

At first pass it looks like everything has been updated properly according to the comments made. I do see noted that the misalignment of the title1 header will be addressed as another issue.

I may be mistaken and I know the images are addressed specifically but I don't see any visual changes made in this issue and am wondering if the images are necessary.

I noticed that there are still some unresolved comments in the issue. I believe that they have all been addressed in your more recent commits but would ask that they be marked as 'resolved' before this issue is merged. You can find the relevant conversations at the bottom of the page:

[trishajjohnson]

@pattshreds I took a look at your pull request and all the requested changes. You effectively added the heading classes, the hierarchy of heading tags have been done properly and upon inspection, it appears in the window everything lines up with the figma design, except the already previously mentioned discrepancies: the h1 at the top in desktop view not being in line with the following paragraph.

I noticed that you also resolved the issue with the h3 from line 26 being improperly indented in mobile view, and it is now in line with the figma design. I wonder, though, not that this is part of your issue, but the h3 on line 26, as it is now, seems awkwardly jetted out significantly and might look better more in line with the following paragraph like the rest of the headings and paragraphs. I also noticed in mobile view that the two paragraphs on lines 11 and 27 have different text-align styles and it might benefit from design continuity (one is centered, the other aligned to the left).

You updated the before/after images, per Jessica's request. I, too, noticed they don't seem different at all and wonder if they are redundant, as Devin did. I also saw that you updated heading styles in _sass/components/_donate.scss.

Overall, great job and kudos to you for sticking with it. Looks like this was a bit of winded issue. Now, you're in the home stretch. Like Devin mentioned, some conversations need to be resolved and then you should be able to merge and close this issue.

[JessicaLucindaCheng]

@pattshreds Sorry for the delay in reviewing your pr as I was on vacation for over a week. You made all the requested changes and everything looks good to me now. Great job!

[JessicaLucindaCheng]

@Sparky-code There actually are visual changes to the font sizes for some of the headers but the changes don't vary much so it may be hard to see by just looking at the webpage.

Also, I am going to go ahead and resolve the conversations since I did not expicitly ask Patrick to "Resolve Conversations" originally in any of my previous reviews and had been resolving them when I reviewed. However, going forward, if you think we should have pr creators Resolve Conversations (if they fixed the problem and there is no more need for conversation) as a standard practice in our team, we can discuss it further.

See this comment

[pattshreds]

@JessicaLucindaCheng Thanks Jessica, I usually do resolve the conversations, but for some reason 2 of the most recent requests didn't have the resolve button on them and I couldn't figure out why.