search

jupyterhub / binderhub

Run your code in the cloud, with technology so advanced, it feels like magic!
https://binderhub.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.57k stars 390 forks source link

html: set meta charset=utf-8 explicitly #1547

Closed damli40 closed 2 years ago

damli40 commented 2 years ago

My first ever Pull request, so excited..yay :)....hope it gets merged

After using the WAVE tool to assess the Page's accessibility, I added the lang="en" in the HTML tag and an alt text to the Binder logo image

imageimage

Reference https://github.com/jupyterhub/outreachy/issues/38

welcome[bot] commented 2 years ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

betatim commented 2 years ago

Hi 👋

thanks for opening a pull request, your first one!

To help review PRs and understand their context it is helpful for PR authors to include a short description of why they made the changes they made and if there are a lot of changes a short summary of them. Generally a good assumption to make is that the people reviewing your PR will know nothing about it (and you) except what is written in the PR itself. SO it is helpful to include some information that helps them make a decision or give advice on what to do next with the PR to move it closer to being merged.

Please also change the title of the PR to something more descriptive.

damli40 commented 2 years ago

Hi 👋

thanks for opening a pull request, your first one!

To help review PRs and understand their context it is helpful for PR authors to include a short description of why they made the changes they made and if there are a lot of changes a short summary of them. Generally a good assumption to make is that the people reviewing your PR will know nothing about it (and you) except what is written in the PR itself. SO it is helpful to include some information that helps them make a decision or give advice on what to do next with the PR to move it closer to being merged.

Please also change the title of the PR to something more descriptive.

Thank you so much, I have made the necessary changes!

consideRatio commented 2 years ago

Thanks @damli40 for your work on this!! :heart: :tada: :sunflower:

There are quite a bit of new things for me to learn related to this!

damli40 commented 2 years ago

Thanks @damli40 for your work on this!! :heart: :tada: :sunflower:

There are quite a bit of new things for me to learn related to this!

  • I understood the alt= part, :+1:

  • I don't yet understand what the .hintrc file does or what tools would be influenced by it. Is it for your use with WAVE? Note that I have never used WAVE and have no knowledge about it.

    If the .hintrc file is to be added, I'd like this PR to include a clarification of what it does and why its added. Ideally the .hintrc file itself should have a comment to clarify its purpose if it supports comments. For example like the .flake8 file has

  • I tried to learn what role="list" was about and found notes about ARIA roles. It seems from this documentation on the <ul> tag that there is an implicit ARIA role="list" already, so maybe we don't need to specify that? I'm not sure, what do you think?

Thank you so much for the response!

All in all, thank you so much, learning and giving knowledge is why I fell in love with open source!

Hopefully, with these corrections the PR can get merged!❤️

minrk commented 2 years ago

Thank you for the pull request!

In general in the future, it's a good idea to separate PRs that add tool configurations from changes to existing files. Usually, if a tool configuration is not present in a project, discussion about adopting the tool in the project should happen first, because as soon as a configuration file is added, it's up to the project maintainers to maintain that configuration file, along with all the others. If we don't actually use the tool, it can easily become stale and its presence misleading.

I've resolved conflicts with another PR that made some of the same changes.

manics commented 2 years ago

https://webhint.io/docs/user-guide/#customize-webhint-in-your-project

A custom hint (.hintrc file) is useful when you want either of the following actions.

Ignore specific errors. Highlight or break for specific warnings.

Does this mean we should automatically run it in CI with a suitable config?

damli40 commented 2 years ago

Thank you for the pull request!

In general in the future, it's a good idea to separate PRs that add tool configurations from changes to existing files. Usually, if a tool configuration is not present in a project, discussion about adopting the tool in the project should happen first, because as soon as a configuration file is added, it's up to the project maintainers to maintain that configuration file, along with all the others. If we don't actually use the tool, it can easily become stale and its presence misleading.

I've resolved conflicts with another PR that made some of the same changes.

Thank you @minrk !!! Finally. This has been a great learning process, all corrections have been noted. Thanks again

minrk commented 2 years ago

@manics

Does this mean we should automatically run it in CI with a suitable config?

I think we will want to get there, yes. Exactly what config to use and how to integrate it in CI is a goal of the outreachy project. It may, for example, require jinja template rendering first.

welcome[bot] commented 2 years ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart: