Skip to content

Conversation

muffinresearch
Copy link
Contributor

@muffinresearch muffinresearch commented May 5, 2016

Fixes: mozilla/addons#9564

Sets-up the UI for the various installation states

Download progress works via generated attribute selectors. We'll need to see how this works in practice - if the perf is bad or we want to reduce the stylesheet weight we could look to manipulating the style properties (not style attributes) via the DOM directly as this won't cause any problems with CSP.

I left out cancelled and error states because it's unclear if we're going to need them on this component. If we need them we can add them in later.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6de755e on muffinresearch:install-buttons-states into 250f844 on mozilla:master.

render() {
const { addonState, downloadProgressPercent } = this.props;
const installed = addonState === INSTALLED &&
addonState !== UNINSTALLED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it should just be addonState === INSTALLED.

@muffinresearch muffinresearch force-pushed the install-buttons-states branch from 6de755e to 9a862ab Compare May 6, 2016 14:45
downloading: isDownloading,
installed,
installing: addonState === INSTALLING,
uninstalling: addonState === UNINSTALLING,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured we'd do something like const switchClasses = switch switch-state-${addonState};.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah or just this would work. I don't have a class for unknown but that doesn't really matter.

const switchClasses =  `switch ${addonState}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make the tests simpler now in light of that change.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9a862ab on muffinresearch:install-buttons-states into 8a51f1a on mozilla:master.

const root = findDOMNode(button);
const checkbox = root.querySelector('input[type=checkbox]');
assert.equal(checkbox.checked, false);
assert.notInclude(root.getAttribute('class'), 'installed');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these would be better off using assert(!root.classList.contains('installed')) since if this had an uninstalled or not-installed or whatever class this would fail.

@mstriemer
Copy link
Contributor

I find downloadProgressPercent and addonState a little verbose but that makes them pretty clear so if you want to shorten them or not that's cool with me.

Looks really good!

r+wc

@muffinresearch muffinresearch force-pushed the install-buttons-states branch from 9a862ab to 6b5ba56 Compare May 6, 2016 15:43
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6b5ba56 on muffinresearch:install-buttons-states into 8a51f1a on mozilla:master.

@muffinresearch
Copy link
Contributor Author

I find downloadProgressPercent and addonState a little verbose but that makes them pretty clear so if you want to shorten them or not that's cool with me.

Yeah I know what you mean. This was something of a conscious decision since state means something in React so I figured that should be distinct. Also I wanted to mention percent because we might be using other units for download progress elsewhere and this makes it super clear if a little long-winded.

@muffinresearch muffinresearch merged commit eef7e3a into mozilla:master May 6, 2016
@muffinresearch muffinresearch deleted the install-buttons-states branch May 6, 2016 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup the install button component to handle the various installation states.
3 participants