using direct link as opposed to popup for render token - #3
Conversation
|
After chatting with Jesse it sounds like creating a direct link as opposed to the current popup is the way to go. Even if I can get the popup to work with keyboard navigation it sounds like it wouldn't be following accessibility guidelines. render_token is used by several pages so this will apply to more than just the accessions view. I think this is better than having a "View" popup as currently implemented |
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
Overall love the change, it's going to solve a lot of the tickets! But I would like to hammer out a few things first.
|
|
||
| html = "<div class='" | ||
| html += "token " if not opts[:inside_token_editor] | ||
| aria_label = CGI.escape_html(strip_tags(clean_mixed_content(opts[:label].to_s))) |
There was a problem hiding this comment.
The ARIA label shouldn't be required if this is a plain link, because the user agent will infer the correct value from the link text.
| link_opts[:class] = "token " unless opts[:inside_token_editor] | ||
| link_opts[:class] = "#{link_opts[:class]}#{opts[:type]}" | ||
| link_opts[:"aria-label"] = aria_label | ||
| link_opts[:tabindex] = 0 |
There was a problem hiding this comment.
This should not be necessary since links are automatically focusable, unless token removes them?
There was a problem hiding this comment.
so remove this whole section (95 - 98) if it's focusable without it and if so ignore the comment below regarding
link_opts[:class] = "#{'token ' unless opts[:inside_token_editor]}#{opts[:type]}"?
| html = "" | ||
|
|
||
| if opts[:icon_class] | ||
| html += "<span class='icon-token #{opts[:icon_class]}'></span>" | ||
| else | ||
| html += "<span class='icon-token'></span>" | ||
| end |
There was a problem hiding this comment.
Since it will unconditionally be started either way, we can remove the initial set to a blank value and have it start with the icon.
| html = "" | |
| if opts[:icon_class] | |
| html += "<span class='icon-token #{opts[:icon_class]}'></span>" | |
| else | |
| html += "<span class='icon-token'></span>" | |
| end | |
| if opts[:icon_class] | |
| html = "<span class='icon-token #{opts[:icon_class]}'></span>" | |
| else | |
| html = "<span class='icon-token'></span>" | |
| end |
| link_opts[:class] = "token " unless opts[:inside_token_editor] | ||
| link_opts[:class] = "#{link_opts[:class]}#{opts[:type]}" |
There was a problem hiding this comment.
On the fence on whether this is better or not. It's slightly less confusing, but it does feel a bit uglier.
| link_opts[:class] = "token " unless opts[:inside_token_editor] | |
| link_opts[:class] = "#{link_opts[:class]}#{opts[:type]}" | |
| link_opts[:class] = "#{'token ' unless opts[:inside_token_editor]}#{opts[:type]}" |
There was a problem hiding this comment.
I actually find the current way a little easier to read
| link_opts = {} | ||
| link_opts.merge!({:target => "_blank"}) if opts[:inside_token_editor] || opts[:inside_linker_browse] | ||
| popover_url = url_for :controller => :resolver, :action => :resolve_readonly | ||
| popover_url += "?uri=#{opts[:uri]}" |
There was a problem hiding this comment.
Since we aren't using a popover any more, the URL variable should probably change name.
There was a problem hiding this comment.
agred, I had been thinking about that too
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
rw+c from @awilfox's comments.
3 participants
Related Ticket (JIRA or GitHub Issue)
ANW-2837
Summary
Updated application_helper so render_token serves a direct link as opposed to a popup. This allows for keyboard navigation. aria_label is also provided for screen readers.
Screenshots (if appropriate):
Types of changes
Checklist: