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

General markup cleanup on buttons and icons #8746

Merged
merged 9 commits into from
Aug 31, 2020
Merged

General markup cleanup on buttons and icons #8746

merged 9 commits into from
Aug 31, 2020

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented Aug 27, 2020

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Curious to see how many places that are still left to convert <a> containing empty href="" attributes to <button> elements I did a search for href="" and noticed that many <button> elements had some leftover empty (and for buttons) invalid href attributes so I just cleaned that up converting them all to type="button" instead.

I also added aria-hidden="true" wherever the buttons wrapped an icon 👼

There was some occurences where an empty href was supplied on links that contained ng-href already so I removed the attribute from there too.

In the "umb-notification" I also removed the aria-hidden="true" attribute from the <i> since the parent element is already hidden for screen readers using that same attribute anyway.

Furthermore I have cleaned up some documentation markup as well so they showcase the use of <button> instead of <a href="">.

All minor stuff really but stuff that is nice to have cleaned up 💯

@bjarnef
Copy link
Contributor

bjarnef commented Aug 28, 2020

@BatJan actually I think the href need to be there since it is required on <a>. It is not an optional attribute. However angular will add/update the href attribute, but until Angular is loaded and the bindings are ready, the markup would be invalid. Maybe not a bit deal in backoffice, but if this was in frontend, the markup wouldn't be valid.

@BatJan
Copy link
Contributor Author

BatJan commented Aug 28, 2020

@bjarnef I appreciate your concern 👍 Since AngularJS make us use loads of invalid stuff like ng-init, ng-hide, ng-*whatever, not to mention all the custom directives the markup we produce in the app is invalid per se in general. And since the href does not serve a purpose in this context it's ok to remove it since we're in a SPA context anyway 😃

It's also not adviced to use empty href's if you build a website with a frontend powered by AngularJS - From the AngularJS docs here you can also see that ng-href="" is the recommended way to do it (Without and empty href="") :-)

@rasmusjp
Copy link
Member

IIRC Safari used to not like it when there's no href, i.e. you could not click the links, not sure if it's still the case

@BatJan
Copy link
Contributor Author

BatJan commented Aug 28, 2020

Doh! It's embarrassing that I could forget this... Without the href on the link will act like a placeholder and therefore loose the baked in accessibility and visuals like cursor pointer (I even mentioned that in my very own Skrift post 😬) - will revert this later. #h5is 🤪

@bjarnef
Copy link
Contributor

bjarnef commented Aug 28, 2020

Also you could have styling target on a:link or a[href]. If you dynamically populate the link and for some reason an error occurs, I believe the styling won't be reflected on the <a> element.

@BatJan
Copy link
Contributor Author

BatJan commented Aug 28, 2020

@nul800sebastiaan The places where I removed href used on ng-href have been reverted - Can't believe I fell into this trap. I probably even might have added some of them myself at an earlier stage 😅 Oh well! Thanks for pointing it out @bjarnef 🙌

@nul800sebastiaan nul800sebastiaan merged commit f33c436 into umbraco:v8/contrib Aug 31, 2020
@nul800sebastiaan
Copy link
Member

Thanks for the feedback all! This cleanup now looks good to me! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants