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.

Sign up for GitHub

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

Jump to bottom

Fix some headings not following the proper hierachy #3965

Closed
3 of 6 tasks
Asartea opened this issue Jul 13, 2023 · 8 comments · Fixed by #3998
Closed
3 of 6 tasks

Fix some headings not following the proper hierachy #3965

Asartea opened this issue Jul 13, 2023 · 8 comments · Fixed by #3998
Assignees
@luuu-xu
Labels
Type: Design Involves the design of the TOP website or requires design review

Comments

@Asartea
Copy link
Contributor

Asartea commented Jul 13, 2023 •
edited
Loading

Complete the following REQUIRED checkboxes:

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this issue follows the brief description of request format, e.g. Add dark mode to website

The following checkbox is OPTIONAL:

  • I would like to be assigned this issue to work on it

1. Description of the Feature Request:
Some headings aren't following proper hierarchy behavior (ie, they aren't properly arranged in sequential descending order). As a spot check I noticed this on the following pages:

  • /about, the text "Help the Odin Project stay current and meaningful to all future students, please contribute!" is a h4. I think the next higher up is a h2, but as the container its in is a direct descendant of the body element I think it would make more sense to convert it to a simple div
  • /faq, all the faq headings are h3 while the "Frequently Asked Questions" itself is h1. All of the h3's should be converted to h2
  • /paths, the "start here" and "PATH" headings are h3's. Again I don't think these necessarily make sense as headings; should just be converted to a div or a p

2. Acceptance Criteria:

  • /about is fixed
  • /faq is fixed
  • /paths is fixed

3. Additional Information:
Could be a decent first issue?
@Asartea Asartea added the Status: Needs Review This issue/PR needs an initial or additional review label Jul 13, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog / Ideas in Main Site Jul 13, 2023
@KevinMulhern KevinMulhern added Status: Help Wanted This issue can be assigned to other contributors and removed Status: Needs Review This issue/PR needs an initial or additional review labels Jul 13, 2023
@KevinMulhern KevinMulhern moved this from 📋 Backlog / Ideas to 🔖 Ready for Development in Main Site Jul 13, 2023
@luuu-xu
Copy link
Contributor

luuu-xu commented Jul 13, 2023

@Asartea Seems like an easy fix, please assign to me so I can submit PR later.

@ManonLef
Copy link
Member

@luuu-xu all yours

@ManonLef ManonLef assigned ManonLef and luuu-xu and unassigned ManonLef Jul 13, 2023
@ManonLef ManonLef moved this from 🔖 Ready for Development to 🏗 In progress in Main Site Jul 13, 2023
@ManonLef ManonLef added Type: Design Involves the design of the TOP website or requires design review and removed Status: Help Wanted This issue can be assigned to other contributors labels Jul 13, 2023
@luuu-xu
Copy link
Contributor

luuu-xu commented Jul 15, 2023

@Asartea I was working on fixes and I felt like

  1. for /about, the original h4 for "Help the Odin Project stay current and meaningful to all future students, please contribute!" should be better be changed to a simple p since it is a text after all?
  2. for /paths, I did see "Start here" is originally h3 and should be a p, but I see the two "PATH" are already p now. So no need to change them right? I'm just double checking with you.
  3. for /contributing, I am not sure whether "There are two main ways you can contribute:" should be a h3? Maybe a h2 is better for it since its higher-up is h1. And "Hall of Fame" definitely needs to be changed from h3 to a h2. What do you think?

@Asartea
Copy link
Contributor Author

Asartea commented Jul 15, 2023

for /about, the original h4 for "Help the Odin Project stay current and meaningful to all future students, please contribute!" should be better be changed to a simple p since it is a text after all?

Yes; however I just noticed this is being generated by app/view/shared/_bottom_cta.html.erb, and messing with that could have farther reaching consequences, especially since it can make sense for it to be a header if a sub heading is present. @KevinMulhern thoughts? My immediate idea is to simply extend the if sub_heading statement to make the h4 a p if no subheader is present.

for /paths, I did see "Start here" is originally h3 and should be a p, but I see the two "PATH" are already p now. So no need to change them right? I'm just double checking with you.

You don't have to touch those at all actually; as mentioned PATH is already a p, and the "Start here" is getting dropped with #3971

for /contributing, I am not sure whether "There are two main ways you can contribute:" should be a h3? Maybe a h2 is better for it since its higher-up is h1. And "Hall of Fame" definitely needs to be changed from h3 to a h2. What do you think?

Makes perfect sense to me

@luuu-xu
Copy link
Contributor

luuu-xu commented Jul 15, 2023 •
edited
Loading

Makes perfect sense to me

I will go ahead and make these changes to /contributing as well then.

My immediate idea is to simply extend the if sub_heading statement to make the h4 a p if no subheader is present.

Let's wait for Kevin's input and I like the idea where if a sub_heading is present, we will have heading to be a h4, or a p if no sub_heading is following it.

@Asartea
Copy link
Contributor Author

Asartea commented Jul 16, 2023

we will have heading to be a h4

Although really, it should be a h2 in that case shouldn't it? Most pages should have at one h1 as the page title, and this logically is a direct descendant of it, because its a stand alone callout

@Asartea Asartea mentioned this issue Jul 16, 2023
Closed
5 tasks
@KevinMulhern
Copy link
Member

Hey @luuu-xu @Asartea, sorry for the delay. I think you're right Asartea, h2 makes the most sense.
It should be fine to have it a h2 in all cases, whether theres a subheading present or not.

@luuu-xu
Copy link
Contributor

luuu-xu commented Jul 17, 2023

@KevinMulhern @Asartea Thank you guys for your input! I will change the heading to h2 then. Creating the PR now.

This was referenced Jul 17, 2023
Closed
Merged
KevinMulhern pushed a commit that referenced this issue Jul 21, 2023
@luuu-xu
Fix: wrong hierarchy for some headings (#3998)
b9ca5f7 
<!-- Thank you for taking the time to contribute to The Odin Project. In
order to get this pull request (PR) merged in a reasonable amount of
time, you must complete this entire template. -->

## Because
<!-- Summarize the purpose or reasons for this PR, e.g. what problem it
solves or what benefit it provides. -->
Some headings of our static pages are not using the correct hierarchy.

## This PR
<!-- A bullet point list of one or more items describing the specific
changes. -->

1. For `/faq` page, changed `components/faq/item_component.html.erb`
heading from `h3` to `h2`.
2. For `/about` page, changed `views/shared/_bottom_cta.html.erb`
heading from `h4` to `h2`.
3. For `/contributing` page, changed two headings's hierarchy.

## Issue
<!--
If this PR closes an open issue in this repo, replace the XXXXX below
with the issue number, e.g. Closes #2013.

If this PR closes an open issue in another TOP repo, replace the #XXXXX
with the URL of the issue, e.g. Closes
https://github.com/TheOdinProject/curriculum/issues/XXXXX

If this PR does not close, but is related to another issue or PR, you
can link it as above without the 'Closes' keyword, e.g. 'Related to
#2013'.
-->
Closes #3965 

## Additional Information
<!-- Any other information about this PR, such as a link to a Discord
discussion. -->

This is a resubmitted PR due to my remote branch conflicts. The previous
PR #3995 is closed.

## Pull Request Requirements
<!-- Replace the whitespace between the square brackets with an 'x',
e.g. [x]. After you create the PR, they will become checkboxes that you
can click on. -->
- [x] I have thoroughly read and understand [The Odin Project
Contributing
Guide](https://github.com/TheOdinProject/theodinproject/blob/main/CONTRIBUTING.md)
- [x] The title of this PR follows the `keyword: brief description of
change` format, using one of the following keywords:
  - `Feature` - adds new or amends existing user-facing behavior
- `Chore` - changes that have no user-facing value, refactors,
dependency bumps, etc
  - `Fix` - bug fixes
-   [x] The `Because` section summarizes the reason for this PR
- [x] The `This PR` section has a bullet point list describing the
changes in this PR
- [x] I have verified all tests and linters pass after making these
changes.
- [x] If this PR addresses an open issue, it is linked in the `Issue`
section
-   [ ] If applicable, this PR includes new or updated automated tests
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Main Site Jul 21, 2023
Mclilzee pushed a commit to Mclilzee/theodinproject that referenced this issue Aug 2, 2023
@luuu-xu @Mclilzee
Fix: wrong hierarchy for some headings (TheOdinProject#3998)
2ae740d 
<!-- Thank you for taking the time to contribute to The Odin Project. In
order to get this pull request (PR) merged in a reasonable amount of
time, you must complete this entire template. -->

## Because
<!-- Summarize the purpose or reasons for this PR, e.g. what problem it
solves or what benefit it provides. -->
Some headings of our static pages are not using the correct hierarchy.

## This PR
<!-- A bullet point list of one or more items describing the specific
changes. -->

1. For `/faq` page, changed `components/faq/item_component.html.erb`
heading from `h3` to `h2`.
2. For `/about` page, changed `views/shared/_bottom_cta.html.erb`
heading from `h4` to `h2`.
3. For `/contributing` page, changed two headings's hierarchy.

## Issue
<!--
If this PR closes an open issue in this repo, replace the XXXXX below
with the issue number, e.g. Closes TheOdinProject#2013.

If this PR closes an open issue in another TOP repo, replace the #XXXXX
with the URL of the issue, e.g. Closes
https://github.com/TheOdinProject/curriculum/issues/XXXXX

If this PR does not close, but is related to another issue or PR, you
can link it as above without the 'Closes' keyword, e.g. 'Related to
TheOdinProject#2013'.
-->
Closes TheOdinProject#3965 

## Additional Information
<!-- Any other information about this PR, such as a link to a Discord
discussion. -->

This is a resubmitted PR due to my remote branch conflicts. The previous
PR TheOdinProject#3995 is closed.

## Pull Request Requirements
<!-- Replace the whitespace between the square brackets with an 'x',
e.g. [x]. After you create the PR, they will become checkboxes that you
can click on. -->
- [x] I have thoroughly read and understand [The Odin Project
Contributing
Guide](https://github.com/TheOdinProject/theodinproject/blob/main/CONTRIBUTING.md)
- [x] The title of this PR follows the `keyword: brief description of
change` format, using one of the following keywords:
  - `Feature` - adds new or amends existing user-facing behavior
- `Chore` - changes that have no user-facing value, refactors,
dependency bumps, etc
  - `Fix` - bug fixes
-   [x] The `Because` section summarizes the reason for this PR
- [x] The `This PR` section has a bullet point list describing the
changes in this PR
- [x] I have verified all tests and linters pass after making these
changes.
- [x] If this PR addresses an open issue, it is linked in the `Issue`
section
-   [ ] If applicable, this PR includes new or updated automated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@luuu-xu luuu-xu

Labels
Type: Design Involves the design of the TOP website or requires design review
Projects
Main Site
Archived in project
Milestone
No milestone
4 participants
@KevinMulhern @Asartea @ManonLef @luuu-xu