Feature/alertbox #218
Feature/alertbox #218
Conversation
jlealess
commented
Jun 14, 2020
jlealess
commented
- creates a new alertbox component to encourage users to contribute
- adds alertbox component to definition page template to ask users improve existing definitions
…fined into feature/alertbox
|
@tatianamac I just saw your message about my previous PR being closed when you renamed your branch; this replaces the old one. |
There was a problem hiding this comment.
This looks great to me!
I also noticed the PR also contains a change to package-lock.json. I checked out your branch, did an npm install, and the optionals got added back. I've seen this happen in the past when the developer's local node version doesn't match the project's. In this case, selfdefined seems to use node version 12.
(In case anyone is unaware, nvm can manage your node versions, which gets enforced by the project's .nvmrc file.)
| @@ -0,0 +1,4 @@ | |||
| <div class="alertbox"> | |||
| <h2 class="alertbox__title">Something not quite right?</h2> | |||
| <p class="alertbox__content">If you found a typo, would like to add another resource, or add nuance to this definition, please feel free to submit <a href="https://github.com/tatianamac/selfdefined/issues" rel="noreferral">an issue</a> or <a href="https://github.com/tatianamac/selfdefined/pulls" rel="noreferral">pull request</a>.</p> | |||
There was a problem hiding this comment.
noreferral may not be a valid rel value; maybe it's meant to be noreferrer or nothing?
I also had this thought, but feel free to ignore: Since the links' intent are for actions the user could take, what do you think of adding a verb to each link? This might be helpful when visually scanning content, or for generating a list of links (e.g. in a web rotor), as it's clearer what the link is for. In this case, this would read, "... feel free to [submit an issue] or [submit a pull request]".
There was a problem hiding this comment.
Hey @sarenji , these are great suggestions!
Good catch on noreferral. I copied the links from the ones already in /selfdefined/11ty/index.njk without thinking about the syntax. I don't have any background on why noreferral would have been used as the rel value for those links but operating under the assumption that there's a reason that file's author put it there and that they actually wanted noreferrer, I've updated the syntax there as well as in my file.
I've taken your suggestion about including the action verbs in the links as well; it reads and scans better. Thanks!
There was a problem hiding this comment.
The author was me and the reason was «not really thinking». Thanks for fixing it. If you like, feel free to add them to other external links, too. If not, no worries, I’ll open a separate PR :)
There was a problem hiding this comment.
Hey @olvb , thanks for the context!
I don't mind making that update but I'm thinking it might be good to address updating other external links in a separate PR so that we can get this callout into prod after being in the works for a few weeks and keep this PR lean.
If I were to submit a separate PR for that, should all external links have rel=noreferrer applied to them?
There was a problem hiding this comment.
Agreed with not cluttering this PR. I’ve created #236 to have a dedicated space for this discussion and to agree on an outcome which we can implement.
There was a problem hiding this comment.
(Oh, btw, my username is ovlb, olvb is someone totally unrelated, but I think by now they have muted this repo, because this typo happens so often.)
|
Thank you for reviewing, @sarenji ! 🙏🏽 |
jlealess
force-pushed
the
feature/alertbox
branch
from
fef1792 to
159b636
Compare
|
@tatianamac Yes, I've implemented @sarenji 's excellent suggestions and updated my branch; it's ready for review again. |
|
@jlealess Awesome! I have merged this in! Would you be okay with thanking you for contributing this when we share it on Twitter? |
