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

Rails: Update link texts to improve accessibility #28293

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

mathdebate09
Copy link
Contributor

Because

Revise the link texts as per the instructions given in the issue.

This PR

Issue

Related to #28290

Additional Information

nil

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project curriculum contributing guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@github-actions github-actions bot added the Content: Ruby on Rails Involves the Ruby on Rails course label Jun 28, 2024
@mathdebate09
Copy link
Contributor Author

Should I fix the linting issues as well? The issue mentioned to let them be

@MaoShizhong
Copy link
Contributor

For these, I can trust you to address any reasonable lint errors. Most should be fixable using the appropriate npm fix scripts, then if any obvious ones remain, you can manually fix those.

Feel free to ask any questions if you encounter any less clear lint errors.

@mathdebate09
Copy link
Contributor Author

mathdebate09 commented Jun 28, 2024

Yes they were really easy fixes, but the linter still screams, it requires a section on lesson overview, can i work on it as well? Also there's an error which didn't make sense to me

It's a Knowledge Check section which is always written with h3, should i change it to h4 as per the linter?

Edit: I forgot to ask for another change i made, in the ruby_on_rails/mailers_advanced_topics/websockets_and_actioncable.md file, there were clashing headers with both having title as Connections (lines 63 and 104), I've made them a bit more descriptive and thus no clashing links and updated link in Knowledge Check as well.

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

We should leave the lesson overview errors alone - those require much more thought and won't be appropriate to resolve in this PR.

The error regarding ### Knowledge checks is because the heading is called Knowledge checks when it should be Knowledge check. De-pluralising checks will let the linter recognise it as the appropriate section, thus allowing the h3.

Regarding the two changed headings in the websockets lesson, what you have I believe should be fine. You will need to use sentence case though, so they should be changed to WebSocket connections and Client-server connections.

@mathdebate09
Copy link
Contributor Author

I believe this should do it, also I moved the Conclusion up, right before Assignments section to adhere to the lesson structure.

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

🚀

@MaoShizhong MaoShizhong merged commit c8fd5a6 into TheOdinProject:main Jun 28, 2024
2 of 3 checks passed
@mathdebate09 mathdebate09 deleted the rails_link_update branch June 28, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: Ruby on Rails Involves the Ruby on Rails course
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants