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

ClickableComponent vs. Button - do we still need to support both components? #3837

Open
misteroneill opened this issue Dec 6, 2016 · 6 comments
Labels
needs: discussion Needs a broader discussion pinned Things that stalebot shouldn't close automatically question

Comments

@misteroneill
Copy link
Member

misteroneill commented Dec 6, 2016

@OwenEdwards brought up a good point in #3828:

There were still a couple of components which are clickable divs instead of buttons - one of them was the poster image (I don't remember the other). I'd been discouraged from making DOM changes, so I made this split to maintain DOM backward-compatibility while fixing problems in the JavaScript and allowing menu buttons to work correctly. Ultimately (in v6.0?) the clickable-component and the button components should be merged again, and the option to pass a tag removed.

I'd like to propose that in 6.0 we:

  1. Deprecate ClickableComponent and remove it from the inheritance chain wherever it exists, but leave it in video.js in case it's being used externally.
  2. Make the Button component not accept a tagName argument at all (a slight deviation from the current state of Remove deprecation for a Button created with a non-button element #3828).
@brandonocasey
Copy link
Contributor

brandonocasey commented Dec 13, 2016

button no longer accepts a tagName as of the most recent commit in PR #3828 button ignores the tagName now in #3828

@OwenEdwards
Copy link
Member

So #3828 addresses 2. above; will 1. go into v6, and if so will we change any components which still inherit from ClickableComponent to inherit from Button?

@OwenEdwards
Copy link
Member

@gkatsev is deprecating ClickableComponent in favor of Button something that could go into v7? Or maybe v8? There are still a couple of components which inherit from ClickableComponent, especially MenuItems, but that could be fixed. It would have an impact on the HTML structure, though, so it's a major change.

@gkatsev
Copy link
Member

gkatsev commented May 9, 2018

we can probably deprecate it in v7 and then look to remove it in v8.

@OwenEdwards OwenEdwards changed the title ClickableComponent/Button in 6.0 ClickableComponent vs. Button - do we still need to support both components? May 9, 2018
@mister-ben
Copy link
Contributor

mister-ben commented May 10, 2018

Would that make the poster and menu items buttons? What's the advantage vs role="button" or role="menuitem"?

@OwenEdwards
Copy link
Member

@mister-ben the menu-items could definitely become buttons; I don't know about the poster - I'd need to take a look at whether it could become a button element or not.

Before v5, a lot of the controls were not <button>s - they were <div>s or <span>s with onclick handlers. But this doesn't give keyboard operation or full screen reader compatibility of the controls (without some additions, e.g. onkeypress, tabindex, an ARIA role and a textual label). After much discussion (#291, #1499 & #1932, amongst others) controls were switched over to <button> elements, and the Button component could accept a tag type for the exceptions. But there were several problems with this approach, so I split Buttons and ClickableComponents; in some ways Buttons naturally extend ClickableComponents, but in some ways they don't, so the inheritance model is kludgy. Moving forward, it would be cleaner to drop the concept of a clickable component which isn't a button (but there may need to be exceptions - the poster image is the one that I'm thinking could possibly be an exception).

@gkatsev gkatsev added the pinned Things that stalebot shouldn't close automatically label Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion Needs a broader discussion pinned Things that stalebot shouldn't close automatically question
Projects
None yet
Development

No branches or pull requests

5 participants