[A11Y] Replace invalid link elements with buttons #3021
Conversation
davwheat
added
the
type/accessibility
|
I haven't reviewed yet, but we have |
|
Unfortunately not. We definitely don't want the underlining in some places (post scrubber for example), but other areas I'd be open to ("links" in modals). |
|
Ah I see, makes sense. btw, I believe we can do |
|
Not tested the changes or really looked at CSS, mainly focused on JS. We could potentially have a < That way it would avoid writing |
| &:focus, | ||
| &.focus, |
There was a problem hiding this comment.
Won't moving these styles from --link to --reset be breaking if the button doesn't update to use --reset with a dropdown toggle? Is it a big deal?
|
For example, the timestamp in posts now uses the primary color. If this happens here, will probs happen with exts - not that big of a breaking change though, so I don't know how much we want that to prevent these changes. Images are before vs after.
|
| @@ -29,9 +30,9 @@ export default class PostMeta extends Component { | |||
|
|
|||
| return ( | |||
| <div className="Dropdown PostMeta"> | |||
| <a className="Dropdown-toggle" onclick={selectPermalink} data-toggle="dropdown"> | |||
| <Button className="Button Button--reset Dropdown-toggle" onclick={selectPermalink} data-toggle="dropdown"> | |||
There was a problem hiding this comment.
the timestamp looks colored at all times, can we instead use Button--text here and just add a vertical-align: bottom; to .Button--text ? it's a lot more consistent.
| <Button class="Button Button--reset" onclick={this.forgotPassword.bind(this)}> | ||
| {app.translator.trans('core.forum.log_in.forgot_password_link')} | ||
| </Button> | ||
| </p>, | ||
|
|
||
| app.forum.attribute('allowSignUp') ? ( | ||
| <p className="LogInModal-signUp">{app.translator.trans('core.forum.log_in.sign_up_text', { a: <a onclick={this.signUp.bind(this)} /> })}</p> | ||
| <p className="LogInModal-signUp"> | ||
| {app.translator.trans('core.forum.log_in.sign_up_text', { | ||
| a: <Button class="Button Button--reset" onclick={this.signUp.bind(this)} />, |
There was a problem hiding this comment.
these look weird as well, colored with no effect on hover, the underline hovering effect of the link was nice.
perhaps we should add a new Button--textlink that would mimic an exact link?
|
Hello, thank you for your pull request! In order to speed up future development, keep all work in one place, and take advantage of CI tools that can help us avoid breaking changes, we have moved all Flarum code to a single monorepo at flarum/framework. You can read more about this process in our dev diary. Unfortunately, pull requests can't be carried over to the monorepo, so we have to close all open pull requests. If this pull request is still relevant, please feel free to reopen it over on the monorepo. We apologize for the inconvenience, and hope you will consider contributing to Flarum again in the future. |
Fixes #3519
Changes proposed in this pull request:
<button>elements.Button--resetclass which removes all native styles from buttons.box-shadow()mixin. It's unneeded and bloats the CSS.Reviewers should focus on:
Anywhere I've missed? Should the Link be removed from "dead" authors?
Screenshot
Confirmed