-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$250] Non-editable rows on the workspace profile show a hover state #54090
Comments
Triggered auto assignment to @isabelastisser ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Non-editable rows on the workspace profile show a hover state What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
!shouldRemoveBackground &&
StyleUtils.getButtonBackgroundColorStyle(
getButtonState(shouldRemoveHoverBackground && (focused || isHovered), pressed, success, disabled, interactive),
true,
),
// OR
!shouldRemoveBackground && shouldRemoveHoverBackground &&
StyleUtils.getButtonBackgroundColorStyle(
getButtonState(focused || isHovered, pressed, success, disabled, interactive),
true,
), What alternative solutions did you explore? (Optional) |
Job added to Upwork: https://www.upwork.com/jobs/~021867410578187381937 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @akinwale ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Non-editable rows on the workspace profile show a hover state What is the root cause of that problem?If the user is non-admin,
App/src/components/MenuItem.tsx Line 619 in 29d546b
Lines 1367 to 1375 in 29d546b
and we don't disable the hover style here if App/src/components/MenuItem.tsx Line 622 in 29d546b
Then the hover style is still applied for a disabled menu item What changes do you think we should make in order to solve the problem?We should not apply the hover style for the disabled items here, we can add
App/src/components/MenuItem.tsx Line 622 in 29d546b
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?NA What alternative solutions did you explore? (Optional)We can use App/src/pages/workspace/WorkspaceProfilePage.tsx Lines 208 to 213 in 29d546b
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Non-editable rows on the workspace profile show a hover state What is the root cause of that problem?MenuItems here are made disabled instead it should be in-interactive. And side Effects of App/src/pages/workspace/WorkspaceProfilePage.tsx Lines 203 to 213 in 29d546b
What changes do you think we should make in order to solve the problem?Use appropriate props here, Pass workspace Title ++ interactive={!readOnly}
-- disabled={readOnly}
-- shouldGreyOutWhenDisabled={false}
-- shouldUseDefaultCursorWhenDisabled What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Non-editable rows on the workspace profile show a hover state What is the root cause of that problem?
App/src/pages/workspace/WorkspaceProfilePage.tsx Lines 203 to 213 in 29d546b
What changes do you think we should make in order to solve the problem?We need to solve a bug with interactive first. When we press the menuitem when App/src/components/MenuItem.tsx Line 611 in 29d546b
Change this to activeOpacity={!interactive ? 1 : variables.pressDimValue} Now, use Diff below ++ interactive={!readOnly}
-- disabled={readOnly}
-- shouldGreyOutWhenDisabled={false}
-- shouldUseDefaultCursorWhenDisabled Also, we can remove the What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?NA since this is an UI change. What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.The non-editable workspace field has a hovered style even though it's disabled. What is the root cause of that problem?When the item is disabled, we already return an empty style from App/src/components/MenuItem.tsx Lines 618 to 622 in e27c5ea
What changes do you think we should make in order to solve the problem?I want to propose a different approach which is more complex but I think is cleaner. The goal is to remove this long line of conditions. App/src/components/MenuItem.tsx Line 622 in e27c5ea
To remove it properly, we need to go back first to find out why it was added. It started on this issue where the blockquote color is the same as the MenuItem hover color, so we want to use a different color. This PR then changes the color inside So, a new PR is created to add the hover style manually.
But it causes another regression since the non-interactive MenuItem still received the hover style. So we patch it by adding After several patches, we end up with this: App/src/components/MenuItem.tsx Line 622 in e27c5ea
Since we want to have a different BG color (only for MenuItem) for HOVERED and FOCUSED, we can separate the ACTIVE button state to HOVERED and FOCUSED. Here are the steps to fix this:
App/src/components/MenuItem.tsx Lines 618 to 619 in e27c5ea
App/src/components/MenuItem.tsx Lines 697 to 698 in e27c5ea
and etc.
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?I think we can test |
After reviewing the proposals here, we can move forward with @bernhardoj's proposal which provides a comprehensive approach to solving the issue, and would prevent further patching / workarounds down the line. 🎀👀🎀 C+ reviewed. |
Triggered auto assignment to @Julesssss, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Sorry @akinwale, but what problem does selected proposal solve? It does the logic refactoring but it does not solve the main issue. And what about this issue
|
@akinwale, @Julesssss, @isabelastisser Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Hey @akinwale, would you mind reviewing @shubham1206agra's comment about proposals? thanks |
Going with the refactor route would help prevent the wrong states from being applied to the menu item which would solve the main issue, and it effectively improves the maintainability of the code to prevent further patching or workarounds in the future. cc @Julesssss |
Sorry @akinwale, but my question still stands. How does this refactoring solve the current issue? I understand the proposal tries to split states for hovered and focused. But it does not solve the root issue since this is an incomplete proposal, and we should not undertake such refactoring without advise from design team. Plus, no other proposal offers any workaround. It just suggests usage of correct params, which selected proposal does not. Plus, you are opening yourself for tons of regression.
This is still not answered yet. |
The refactor introduces better handling of the combination of styles which would also prevent the hover styles from being applied when the menu item is disabled. It is also mostly geared towards code, not design. Being wary of regressions shouldn't be a hindrance to improving the codebase. I do agree however, that the proposal does not completely solve the issue.
I initially missed this. Since the bug is related to the interactive prop affecting the active opacity of the menu item, it looks like your solution addresses the underlying cause. @Julesssss We can move forward with @shubham1206agra's proposal here. |
This isn't a problem in my proposal since we don't use |
This is even more problematic since this introduces new design interface. |
Can you point out which one? I don't think there is any new interface is added from my proposal |
You just changed the design of how disabled works. Disabled does not mean non-interactive. |
I don't think I changed the design, instead, I fixed it. Why should a disabled pressable show a hover style? If you use PressableWithFeedback and apply hoverDimmingValue < 1, the hover won't be applied if it's disabled. Before #43987, disabled MenuItem won't show the hover, but after the PR, now the hover is applied. So, #43987 is the one that changes it, and my proposal will fix it. |
Sorry, but then what is the difference between non-interactive and disabled fields? |
There isn't a design guideline about that, but looking at the code, |
In what way? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
FYI, I will be OOO from Dec 23 to Jan 6, so please reassign the issue to another team member for urgency, IF needed. |
@akinwale, @Julesssss, @isabelastisser Eep! 4 days overdue now. Issues have feelings too... |
@akinwale, @Julesssss, @isabelastisser Still overdue 6 days?! Let's take care of this! |
@akinwale @Julesssss @isabelastisser this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@akinwale, @Julesssss, @isabelastisser 10 days overdue. Is anyone even seeing these? Hello? |
@akinwale, @Julesssss, @isabelastisser 12 days overdue now... This issue's end is nigh! |
I prefer @bernhardoj's button logic improvements in the long-term. But I agree it should be a wider choice we make in Slack. I would support you in making this proposal in Slack and we can tag the design team as an improvement. For now though, lets stick with the fix from @shubham1206agra |
📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR is in progress |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.75-6
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @dannymcclain
Slack conversation (hyperlinked to channel name): expensify_bugs
Action Performed:
Prerequiste: Invite User B as employee to User A's Workspace
Expected Result:
No hover state as the fields are not editable
Actual Result:
Non-editable rows on the workspace profile show a hover state
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
CleanShot.2024-12-12.at.08.23.17.mp4
Recording.845.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @akinwaleThe text was updated successfully, but these errors were encountered: