Feature/gh 1596/rewards icon simple view #407
Conversation
…ng them in line with Zeplin specs
…e it. Convert rewards panel header to use it.
…tive states for Detail component menu icons
| * Show the Rewards view | ||
| * Used to handle user clicking on the Rewards icon | ||
| */ | ||
| showRewardsView() { |
IAmThePan
Jul 2, 2019
Contributor
Do you use this or do you rely on the NavButton link? I'm noticing a bug when I click the Rewards button but not the SVG. It takes me to a blank Detail View.
Do you use this or do you rely on the NavButton link? I'm noticing a bug when I click the Rewards button but not the SVG. It takes me to a blank Detail View.
wlycdgr
Jul 3, 2019
Author
Member
Fixed showRewardsView (renamed to showRewardsList) to correctly hanle click and convert the NavButtons to ReactSVGs to simplify & give showRewardsList sole responsibility.
Fixed showRewardsView (renamed to showRewardsList) to correctly hanle click and convert the NavButtons to ReactSVGs to simplify & give showRewardsList sole responsibility.
IAmThePan
Jul 9, 2019
Contributor
Please revert to use the NavButton. That way worked if you had only removed the onClick handler. This new implementation introduces two bugs:
- There is no
cursor: pointer on the SVG. There should be because it makes sense as you can click the SVG and also because the Historical Stats link above has it.
- You can click the box around the SVG while you can't for Historical Stats.
Please make these as similar as possible by reverting to the old way of using NavButton (like the Historical Stats button does) and by removing the onClick handler (which again, the Historical Stats button doesn't have).
Please revert to use the NavButton. That way worked if you had only removed the onClick handler. This new implementation introduces two bugs:
- There is no
cursor: pointeron the SVG. There should be because it makes sense as you can click the SVG and also because the Historical Stats link above has it. - You can click the box around the SVG while you can't for Historical Stats.
Please make these as similar as possible by reverting to the old way of using NavButton (like the Historical Stats button does) and by removing the onClick handler (which again, the Historical Stats button doesn't have).
wlycdgr
Jul 16, 2019
Author
Member
I've made the UIs and implementations of the Historical Stats and Rewards navicons consistent, but by converting the Stats navicon to use ReactSVG also instead of reverting to NavButton for the Rewards navicon. I chose this solution because NavButton isn't sufficient to handle clicks on the Rewards navicon, which need to call toggleExpert in addition to pushing a new route onto the history stack. I think letting user click anywhere inside the bordered navicon area is, anyhow, more user-friendly than requiringt them to click on the icon itself. It's also more consistent with the click-on-donut behavior (clicking on empty space inside the donut works as well as clicking the donut itself). The cursor behavior is now consistent between the two navicons also.
I've made the UIs and implementations of the Historical Stats and Rewards navicons consistent, but by converting the Stats navicon to use ReactSVG also instead of reverting to NavButton for the Rewards navicon. I chose this solution because NavButton isn't sufficient to handle clicks on the Rewards navicon, which need to call toggleExpert in addition to pushing a new route onto the history stack. I think letting user click anywhere inside the bordered navicon area is, anyhow, more user-friendly than requiringt them to click on the icon itself. It's also more consistent with the click-on-donut behavior (clicking on empty space inside the donut works as well as clicking the donut itself). The cursor behavior is now consistent between the two navicons also.
| return ( | ||
| <div className={rewardsIconClassNames} onClick={this.showRewardsView}> | ||
| <NavButton path="/detail/rewards/list" imagePath="../../app/images/panel/rewards-icon.svg" /> | ||
| {unreadOffersAvailable && <NavButton classNames="Summary__rewardsIcon__star" path="/detail/rewards/list" imagePath="../../app/images/panel/purple-star.svg" />} |
IAmThePan
Jul 2, 2019
Contributor
I haven't been able to test this because I don't know how to generate a reward...
I haven't been able to test this because I don't know how to generate a reward...
wlycdgr
Jul 3, 2019
Author
Member
As per Vinnie's decision about this, the button should display regardless of whether any rewards (new or seen) are available, so long as the user has not turned Rewards off, so it should be enough to check that the icon takes the user to the Rewards list view, and that it does not display when Rewards are off. For testing the purple star appearance / logic, see next comment.
As per Vinnie's decision about this, the button should display regardless of whether any rewards (new or seen) are available, so long as the user has not turned Rewards off, so it should be enough to check that the icon takes the user to the Rewards list view, and that it does not display when Rewards are off. For testing the purple star appearance / logic, see next comment.
| text-transform: capitalize; | ||
| } | ||
| // position the star SVG relative to the gift box SVG | ||
| .Summary__rewardsIcon__star { |
IAmThePan
Jul 2, 2019
Contributor
again, I'll check this when changes are made and I generate a reward.
again, I'll check this when changes are made and I generate a reward.
wlycdgr
Jul 3, 2019
•
Author
Member
You can force the value of unreadOffersAvailable in Summary#_renderRewardsIcon to true to test the appearance/layout of the star. That bool is true wwhen the unread_offer_ids array sent from the background is not empty, and you can stub that array in PanelData#_getPanelData if you want to verify the transmission from background to the Summary component.
You can force the value of unreadOffersAvailable in Summary#_renderRewardsIcon to true to test the appearance/layout of the star. That bool is true wwhen the unread_offer_ids array sent from the background is not empty, and you can stub that array in PanelData#_getPanelData if you want to verify the transmission from background to the Summary component.
|
Hey, there are a few minor things and one minor bug. After those are fixed I'll test the ones with a reward. |
…e them and update their snapshots.
…sponsibility for click handling to Summary#showRewardsList
Implement a Rewards icon in the simple Summary view as per GH-1596