Add a 'download' property to Button #39088
Conversation
And restrict it to only work when the component is rendering an anchor. Intended for use with [Lesson Plan PDFs](#39011) See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-download
| ); | ||
| assert.equal(wrapper.find('a').prop('download'), true); | ||
|
|
||
| wrapper = shallow( |
There was a problem hiding this comment.
Mind splitting these into two tests? One with the anchor and the other without?
The two scenarios seem pretty separate, but happy to have my mind changed :)
There was a problem hiding this comment.
I thought about writing two tests, but decided to be lazy :) I'll go ahead and un-lazy this
| @@ -256,6 +266,7 @@ class Button extends React.Component { | |||
| href={disabled ? 'javascript:void(0);' : href} | |||
| target={target} | |||
| disabled={disabled} | |||
| download={download} | |||
There was a problem hiding this comment.
Will the new functionality be added to existing elements or only to new elements? If it's the latter, it would be better to add this to a new component so we can start moving toward <a> styling rather than <button> styling.
There was a problem hiding this comment.
I'm not sure I follow. Do we already have a component that applies styling to anchor tags, or are you suggesting I split out that functionality entirely? If the latter, that seems like a significant task all on its own.
There was a problem hiding this comment.
Currently, Button is a catch-all component for "something clickable." This presents significant accessibility issues as the styling displays an element that looks like an html button but could be a button, a div (un-noticeable by screen readers) or an a (interpreted differently by screen readers). If this is something we don't use now, but plan to use widely, it should be split into its own component. cc @epeach
There was a problem hiding this comment.
I agree that it's an overcomplicated component, but I think the problem is that it's too easy for us as developers to use it incorrectly and to render into the browser an HTML element that's not suited for the mode of interaction we're using. All of the elements this component is capable of rendering are suitable for different modes of user interaction, we just need to make sure that we're being intentional about what we're doing.
For example, enforcing that only anchor tags support the download property.
There are lots of ways this component could be reworked to make it more obvious to developers how to use it correctly, and that's something we should be tracking and scheduling as cross-team tech debt work. But until we are able to do that work, we do still need to be able to use the functionality we currently have.
There was a problem hiding this comment.
Not to make this thread a party, but I'm interested in seeing if @madelynkasula (owning the component library) has an opinion on if there is an inflection point where we want to move away from previous development patterns and move towards the way we want to do them in the future?
(Edit - I was on a stale version of this thread and just saw Elijah's latest comment - for what it's worth, I agree that the technical work to break this button component apart into more semantically correct parts is out of scope for this PR.)
There was a problem hiding this comment.
I think Elijah's approach makes the most sense for where we're at right now. The a tag is deprecated in Button, but it will be easier for us to refactor this component if everyone is using it so that it can inform how we break this component into more semantically-correct components. (E.g., there will likely be a <Link/> that accepts download as a prop, but <Button/> will not.)
We will hit an inflection point at some point, but I think we still have some decisions to make before we get there. The component library is still in very early planning stages, but this component is very high on my list of initial components for the library for the reasons you all have brought up.
There was a problem hiding this comment.
I'm definitely looking forward to the component library work! There's a lot of potential for some real big wins there
And restrict it to only work when the component is rendering an anchor.
Intended for use with Lesson Plan PDFs
See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-download
Testing story
Added a basic unit test
PR Checklist: