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

Fix CI by removing <a> and <button> nesting as well as ignoring some working links #789

Merged
merged 3 commits into from
Apr 9, 2025

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Apr 9, 2025

This fixes a failing CI due to a rule that we can't nest a button inside of an a element. It also adds two ignore rules for links that were returning errors even though they were valid (and these are common / stable links so I think it's safe to ignore them).

@choldgraf
Copy link
Collaborator Author

OK it looks like our CI has rules that says we can not nest an <a> inside a button, and we cannot nest a <button> inside an <a>. I've changed this to just use <a> and no buttons.

@choldgraf choldgraf changed the title Nest a inside button element Fix CI by removing <a> and <button> nesting as well as ignoring some working links Apr 9, 2025
@@ -90,6 +90,8 @@ jobs:
--check-links-ignore "https://jupytercon.com" \
--check-links-ignore "https://www.netapp.com" \
--check-links-ignore "https://github.com/[^/]+/?$" \
--check-links-ignore "https://sloan.org" \
--check-links-ignore "https://www.bloomberg.com" \
Copy link
Member

Choose a reason for hiding this comment

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

This one actually does not work for me. It redirects to https://bloomberg.com

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for me, going to "bloomberg.com" redirects to "www.bloomberg.com" 🤷

cursor: pointer;
div.con-buttons {
margin-top: 2em;
a.con-button {
Copy link
Member

Choose a reason for hiding this comment

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

Is this acceptable support level https://caniuse.com/css-nesting - 87.4% of global users?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't this SCSS getting compiled to CSS by Jekyll in the actual HTML output?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right!

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thanks so much @choldgraf!

It would be great to fix lighthouse one day too, though it is annoying that it randomly starts failing after few months due to CI being CI.

@choldgraf choldgraf merged commit 6a9488f into main Apr 9, 2025
4 of 5 checks passed
@choldgraf
Copy link
Collaborator Author

thanks - I'll merge since this is an improvement over what's there.

I opened this PR to see if there's an easy fix for lighthouse, let's see

@krassowski krassowski deleted the fix-ci branch April 10, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants