Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fixes #4791 - Update Favorite button on MyShots Page #4918

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

punamdahiya
Copy link
Contributor

No description provided.

@punamdahiya punamdahiya changed the title Fixes #4791 - Update Favorite button on MyShots Page [Hold] Fixes #4791 - Update Favorite button on MyShots Page Sep 19, 2018
Copy link
Collaborator

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

Not sure I'm sold on the "does expire" (I'd prefer "will expire"), but that's a product decision. No issue on the l10n bits.

</g>
</g>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to run the new svgs through svgo.

Copy link
Contributor Author

@punamdahiya punamdahiya Sep 19, 2018

Choose a reason for hiding this comment

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

Done

@chenba
Copy link
Collaborator

chenba commented Sep 19, 2018

The button text color on My Shots need to be updated. And according to #4791 (comment) the icons have been updated, so we should pull those in.

Copy link
Collaborator

@chenba chenba left a comment

Choose a reason for hiding this comment

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

Needs svgo and button text color update.

@punamdahiya punamdahiya changed the title [Hold] Fixes #4791 - Update Favorite button on MyShots Page Fixes #4791 - Update Favorite button on MyShots Page Sep 19, 2018
@punamdahiya
Copy link
Contributor Author

@chenba PR is updated with new UX for favorite icon on my shots and shot page. Please review. Thanks!


favoriteShotButton = <div className="favorite-shot-button"><Localized id="shotPagefavoriteButton">
<button className={`nav-button ${shouldShow} `} onClick={this.onClickFavorite.bind(this)}>
<button className={`nav-button ${shouldDisable} `} onClick={this.onClickFavorite.bind(this)}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use the disabled attribute, not a class.

Copy link
Collaborator

@chenba chenba left a comment

Choose a reason for hiding this comment

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

An element should be disabled via its disabled attribute.

@punamdahiya
Copy link
Contributor Author

PR updated with feedback, thanks

@chenba chenba merged commit 8b84794 into mozilla-services:master Sep 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants