fix: [M3-7400] - Linode Network Transfer History - Back button incorrectly disabled #9900
Conversation
bnussman-akamai
added
Bug
bnussman-akamai
requested review from
cpathipa and
cliu-akamai
and removed request for
a team
…correctly appearing to be disabled
| > | ||
| <ArrowBackIosIcon | ||
| sx={{ | ||
| ...(monthOffset === minMonthOffset |
There was a problem hiding this comment.
This was the main bug 🐛. minMonthOffset should be maxMonthOffset here.
The easy fix would be to change minMonthOffset ➡️ maxMonthOffset but I decided to go ahead and switch these one-off styled buttons to IconButtons with color of primary. This basically recreates the same style, just in a more maintainable way
There was a problem hiding this comment.
Nice fix and good catch with the disabled-but-still-clickable back button in prod. 🔍
With a linode older than 30 days, I confirmed the back button was enabled as expected until I reached the month of its creation. Styles looked good in both light and dark mode.
| </Box> | ||
| </Box> | ||
| {renderStatsGraph()} | ||
| </div> | ||
| ); | ||
| }); | ||
|
|
||
| const sxArrowIconDisabled = (theme: Theme) => ({ | ||
| cursor: 'not-allowed', |
There was a problem hiding this comment.
I'm okay with losing this cursor style because the disabled arrow seems clear enough without the 🚫 cursor and I think we only change the cursor on disabled buttons in a handful of places, but wanted to point out it is a change in case it wasn't intended or anyone has other thoughts.
There was a problem hiding this comment.
I'm in line with showing a 🚫 cursor on actionable items. IMO, it adds an additional layer of visual clarity and facilitates international understanding, indicating that the actionable item is disabled for some reason. This is unless we choose the design option of removing the 🚫. I think this could be a good topic for our cafe meeting to improve consistency.
There was a problem hiding this comment.
My fault for not calling out the change in the PR description. We did lose the not-allowed cursor.
I liked the cursor effect, but I hate to write one-off styles for it... That's why I was okay with removing it. Open to re-adding it
There was a problem hiding this comment.
Since we're not consistent about it across the app already, I think it's not a huge deal to remove it here. But I do think @cpathipa has a good point that this could be worth discussing with UX in cafe to see if we want to apply this cursor treatment more consistently. If there's a consensus of 'yes' that we should, another tech story ticket can revisit this and other places.
Description 📝
Fixes back button disabled state on the Linode Details network tab
Preview 📷
How to test 🧪
Prerequisites
Reproduction steps
Verification steps
As an Author I have considered 🤔