added anchors to guides and external resources

[Carlos-A-P]

Fixes #2802

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

• Added an anchor to the Guides section so that https://www.hackforla.org/toolkit#guides will jump to that section
• Added an anchor to the External Resources section so that https://www.hackforla.org/toolkit#external-resources will jump to that section
• No additional SCSS classes were needed but I did add padding-top to toolkit__card-list for some viewport widths since the logo covered the section titles when the user jumps to the anchors.

Screenshots of Proposed Changes Of The Website

Visuals before changes are applied

Visuals afterchanges 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 Carlos-A-P-add-anchors-to-guides-2802 gh-pages
git pull https://github.com/Carlos-A-P/website.git add-anchors-to-guides-2802

[Sparky-code]

Avail: 1hr
ETA: 4/21-22 EOD

[tamara-snyder]

Availability: 1 hour
Review ETA: 04/21/2022

[tamara-snyder]

Hi @Carlos-A-P, great work on this! All looks good with the anchor tags and thank you for taking the time to fix some of the formatting in this file. I'm just requesting a few changes, which you'll see in my comments. Thanks!

[Sparky-code]

Updated ETA: 4/24/EOD
Blocker: School work + Docker Trouble

[Carlos-A-P]

Hey @tamara-snyder. I have the prettier extension installed, and since I reinstalled my code editor the extension was reenabled for all of my projects and I didn't notice before I pushed... If the formatting looks good, I'll fix up those changes!

[Sparky-code]

Hey @Carlos-A-P Everything looks good across devices. it looks like you've addressed Tamara's formatting requests in your second commit. I'd only ask that you check off the additional steps in the referenced issue (and note if no additional SCSS was added) in your initial comment.

Not part of this issue but something I thought was worth noting here. The 'suggest' buttons don't do anything, it might be worth creating a 'coming soon' onclick class until they have a direction. This could be a useful reusable class for live pages that are under development.

[Carlos-A-P]

Hey @Sparky-code, I checked off the additional steps in the referenced issue and noted if additional SCSS was added to my comment.

Just wanted to note here that we discussed the potential of creating a new issue regarding the 'coming soon' onclick class for the 'suggest' buttons during our meetings and to follow up on Slack. Let me know if find anything else for my pr or if you have any other questions!

[Sparky-code]

Hey @Carlos-A-P, Thanks for doing that and noting our discussion. So discussing that potential issue creation with @JessicaLucindaCheng ended up leading to an issue that she was seeing with this issue where at certain screensize/resolution the when landing on the anchors the title text gets obscured by the H4LA logo:

Anchor Issue

I wasn't seeing this issue when I was using dev tools in chrome so I suggest looking at it through firefox where you have more control over the screen size and resolution.

Note: Its ok that the title block goes behind the logo on scrolling but the anchor should resolve at all screen sizes /resolutions so that the title is not obscured when you land there.

The other point would be that I'm guessing you added the 'data-name' to the anchor tag as a result of the about page reference. That seems to be unnecessary so next steps for this issue would be:

• Remove 'data-name' from the anchor tags generated for the toolkits page
• Test page for the visual issue noted above
• Implement adjustments to resolve this issue

[Carlos-A-P]

Hey @Sparky-code, I kept the data attribute since I wasn't sure of its purpose. I looked more into the about page anchors and wondered if you had any idea what the data-name attributes are for? I don't see them being used in the about.js file.

Also, for the blocking problem do you think I should try to make the anchor jump just a little above the resources section for a certain viewport range? That's the only solution I could come up with and will try to look more into it.

[JessicaLucindaCheng]

@Carlos-A-P I don't see a purpose or use of the data-name attribute for the Guides and External Guides anchors, so it is preferred if we don't include code we aren't using. You can test out what happens if that attribute is removed just to make sure but I'm pretty sure the anchors will function without the data-name attribute and we can always add them later if needed.

[Carlos-A-P]

Hey, @tamara-snyder @Sparky-code I made some changes. I would request for reviewers to review again but the request button doesn't show up on my side?

Either way, I made some changes in _toolkit.scss and it picked up some errors for duplicate selectors. I addressed it in another dev's pr #3085 (comment) who's also working on the same page if they can take a look at it but I can do it too if needed.

[Sparky-code]

Hey @Carlos-A-P Will take a look today/tomorrow. Sorry I didn't get back to your questions before you worked on it. I think that makes sense. If there's another element slightly above you can set it to to give it more 'padding' or more directly do that (Note that anchor elements don't directly take padding). Something I just googled that might be useful: https://stackoverflow.com/questions/10732690/offsetting-an-html-anchor-to-adjust-for-fixed-header

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

[Carlos-A-P]

@Sparky-code sounds good! I'll find a better way to add spacing to it instead of using padding on the anchor tag, thanks!

[tamara-snyder]

Hey @Carlos-A-P, thank you for removing the data-name attributes. I also like Devin's idea for the extra padding. All looks good, I'm just going to wait until the SCSS Linter check is resolved to do a proper review.

[SAUMILDHANKAR]

@Carlos-A-P Hi Carlos, we just noticed that the Lint SCSS workflow is failing for your PR. We are considering making a new issue to resolve this. Just wanted to let you know that unless there was any SCSS (that you modified as part of your PR) which might have resulted in this error, there is no need for you to fix this and you can ignore it for this PR. Thank you.

[Carlos-A-P]

Hey @tamara-snyder @Sparky-code, just made a few updates again and this pr is ready for review. Not too sure as to why I'm still not able to request reviewers even though there are changes requested?

[Sparky-code]

Hey @Carlos-A-P Thanks for working on this and being patient on this review. Looks like the issue is mostly solved though I am noticing a couple of things at smaller pages. I think a slightly larger padding should solve the second issue, at first glance I'm not sure what the cause of the first issue is.

Issue with guides bar organization with smaller screens

Issue with resource headers at smaller screens

test


[Carlos-A-P]

Hey @Sparky-code not sure if it's just my side but I'm not able to view the images in your last comment

[Sparky-code]

@Carlos-A-P Thats strange, im not sure why thats happening. Moved them outside of the details, should be visible now.

[Carlos-A-P]

@Sparky-code for the first image, I could only assume that the images disappear since the guide's list filters by changing the URL. For example, to filter for design it would be "hackforla.org/toolkit/#Design" so the anchor tags would interfere which I now see is an issue as well. I'm also not too sure why at 1238px width it didn't work on your side. I tested the anchor in two different browsers and the headers don't seem to be blocked?

[Sparky-code]

Hi @Carlos-A-P adding this update as a follow up from our discussion on Sunday 5/15 - This issue is still ongoing due to an edge case that occurs when accessing the guides and the usage of anchor tags and hrefs appear to conflict and cause issues with what is being displayed and what is supposed to display. Please let me know If I forgot anything here.

[Carlos-A-P]

Hi @Carlos-A-P adding this update as a follow up from our discussion on Sunday 5/15 - This issue is still ongoing due to an edge case that occurs when accessing the guides and the usage of anchor tags and hrefs appear to conflict and cause issues with what is being displayed and what is supposed to display. Please let me know If I forgot anything here.

Hey @Sparky-code, that's correct, and was also wondering if you still get the problem with the logo covering the header after pulling the latest changes in the branch?

[Sparky-code]

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

[Sparky-code]

This appears to solve the issue of the logo overlaying on the section title when screen size was close to the media breakpoint. Nice!

[kathrynsilvaconway]

@Carlos-A-P I can see that the two anchors are added and everything else looks good in the code. Nice job cleaning up the padding as well. It looks like everything unresolved in this discussion is out side of the scope of the original issue, so I am just going to approve it. Very good work.

[tamara-snyder]

Hi @Carlos-A-P, the anchors work great! Thanks for working on this issue.

[JessicaLucindaCheng]

@tamara-snyder @Sparky-code Is this pr ready to be merged?

I know the Lint SCSS check is failing. However, if the linting is catching code issues that Carlos did not edit nor add (meaning the linting is catching something that already exists in the code and Carlos did not touch that portion of the code), it should be okay to merge as long as the linting problem doesn't break anything on the website.

[Sparky-code]

@JessicaLucindaCheng
Yes, in regards to the linting issue.
I do see now that there are merge conflicts that have come up after merging #3085.
I believe the right thing to do here is have @Carlos-A-P rebase his branch and commit from there as I think this issue would have come up even if I'd merged this issue first.

Confirming the Lint failure does not appear to have a negative impact on the page Carlos worked on. Its not failing over code he edited. I re-ran his branch locally to be sure, am adding the error output here for reference for creating an issue to fix this as I believe it has come up in a few issues though I haven't investigated where the duplicate class came from / was made for.

_sass/components/_toolkit.scss
235:1 ✖ Unexpected duplicate selector ".resource-svg-icons", first used at line 120 no-duplicate-selectors
240:1 ✖ Unexpected duplicate selector ".toolkit-info-container", first used at line 126 no-duplicate-selectors
241:5 ✖ Unexpected duplicate selector ".toolkit-info-container h3", first used at line 130 no-duplicate-selectors

[Carlos-A-P]

@Sparky-code, just fixed the merged conflict so it's ready to be reviewed again.

[Sparky-code]

Everything looks good, I see you've resolved the merge conflict by including the changes for the toolkits page that were merged from a separate issue since your previous commit.