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

Use more accessible visual elements instead of buttons #231

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+31 −14
Draft

Use more accessible visual elements instead of buttons #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/_static/try_examples.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
}

.try_examples_button {
text-decoration: none;
display: inline-block;
cursor: pointer;
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

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

Why is this CSS needed?

background-color: var(--jupyter-light-primary);
border: none;
padding: 5px 10px;
Expand Down
6 changes: 6 additions & 0 deletions jupyterlite_sphinx/jupyterlite_sphinx.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,9 @@
display: none;
}
}

a.try_examples_button {
text-decoration: none;
display: inline-block;
cursor: pointer;
}
36 changes: 22 additions & 14 deletions jupyterlite_sphinx/jupyterlite_sphinx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,10 @@ def __init__(

def html(self):
return (
'<button class="try_examples_button" '
f"onclick=\"window.open('{self.lab_src}')\">"
f"{self.button_text}</button>"
f'<a href="{self.lab_src}" '
'target="_blank" rel="noopener noreferrer" '
'class="try_examples_button" style="text-decoration: none; display: inline-block; color: inherit;">'
Copy link

Choose a reason for hiding this comment

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

Aren't these styles duplicates of the stylesheet? I think styling them via a.try_examples_button should be sufficient.

f"{self.button_text}</a>"
)


Expand Down Expand Up @@ -322,9 +323,10 @@ def __init__(

def html(self):
return (
'<button class="try_examples_button" '
f"onclick=\"window.open('{self.lab_src}')\">"
f"{self.button_text}</button>"
f'<a href="{self.lab_src}" '
'target="_blank" rel="noopener noreferrer" '
Copy link

Choose a reason for hiding this comment

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

I'm not entirely sure why you need noopener or noreferrer.

Generally speaking noreferrer is used if you're building something like a discussion forum. It removes the incentive for people to post spam links. Noreferrer says to Google something like: as the webmaster of this site, I disavow this link, it could be spam, please don't penalize my search engine rankings for including it on my site.

The general idea behind both noopener and noreferrer is that you don't know where you're linking to. You're linking to third-party content that you're concerned could potentially abuse your site or visitor via the link. As far as I understand, that's not the same case here because the try_examples link is created from content that you own and produce.

In any case, this is a change that should be in a separate pull request because it's not really relevant to the other changes in this pull request, and it changes the existing behavior.

'class="try_examples_button" style="text-decoration: none; display: inline-block; color: inherit;">'
f"{self.button_text}</a>"
)


Expand Down Expand Up @@ -675,17 +677,23 @@ def run(self):

# Button with the onclick event to swap embedded notebook back to examples.
go_back_button_html = (
'<button class="try_examples_button" '
f"onclick=\"window.tryExamplesHideIframe('{examples_div_id}',"
f"'{iframe_parent_div_id}')\">"
"Go Back</button>"
Comment on lines -678 to -681
Copy link

Choose a reason for hiding this comment

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

I see no reason to change this element. It really is a button; it's not link, it's not taking you to a new URL.

Generally speaking, you should only use the role attribute as a very last resort, in cases where you have constraints that prevent you from using the HTML tag that has the role you need. (For example, imagine you want to fix the semantics of the markup output by a tool, such as Sphinx, that's been around a long time. Imagine that there are a lot of downstream styles and code that have been built around that specific markup structure. Then you cannot safely change an a-tag to a button-tag without breaking a lot of downstream projects. So if you wanted to fix the semantics, your only safe option would be to use the role attribute.)

'<a href="#" '
'role="button" '
'aria-label="Return to original content and close the examples frame" '
f'onclick="window.tryExamplesHideIframe(\'{examples_div_id}\', '
f'\'{iframe_parent_div_id}\'); return false;" '
'class="try_examples_button" '
'style="text-decoration: none; display: inline-block;">'
'Go Back</a>'
)


full_screen_button_html = (
'<button class="try_examples_button" '
f"onclick=\"window.openInNewTab('{examples_div_id}',"
f"'{iframe_parent_div_id}')\">"
"Open In Tab</button>"
f'<a href="{iframe_src}" '
'target="_blank" rel="noopener noreferrer" '
'class="try_examples_button" '
'style="text-decoration: none; display: inline-block;">'
"Open In Tab</a>"
)

# Button with the onclick event to swap examples with embedded notebook.
Expand Down
Loading