-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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] Workspace settings - There is back button on not found page for wide screen #50175
Comments
Triggered auto assignment to @alexpensify ( |
We think that this bug might be related to #wave-collect - Release 1 |
@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
Edited by proposal-police: This proposal was edited at 2024-10-03 17:54:18 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Workspace settings - There is back button on not found page for wide screen What is the root cause of that problem?
App/src/pages/workspace/AccessOrNotFoundWrapper.tsx Lines 90 to 101 in ba9af8f
What changes do you think we should make in order to solve the problem?
What alternative solutions did you explore? (Optional)
fullPageNotFoundViewProps={{
shouldShowBackButton: shouldUseNarrowLayout,
}}
What alternative solutions did you explore? (Optional 2)
fullPageNotFoundViewProps={{
shouldForceFullScreen: true,
}} What alternative solutions did you explore? (Optional 3)
function isPolicyFeatureEnabled(policy: OnyxEntry<Policy>, featureName: PolicyFeatureName | 'moreFeatures', currentUserLogin?: string): boolean {
if (featureName === CONST.POLICY.MORE_FEATURES.ARE_TAXES_ENABLED) {
return !!policy?.tax?.trackingEnabled;
}
if (featureName === CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED) {
return policy?.[featureName] ? !!policy?.[featureName] : !isEmptyObject(policy?.connections);
}
if (featureName === 'moreFeatures') {
return isPolicyAdmin(policy, currentUserLogin) && isPaidGroupPolicy(policy);
}
return !!policy?.[featureName];
}
NOTE: We would also need to solve type issues when we use Result |
Proposal Updated
|
Edited by proposal-police: This proposal was edited at 2023-10-06T13:45:00Z. ProposalPlease re-state the problem that we are trying to solve in this issue.There is back button on not found page for wide-screen What is the root cause of that problem?We display a full-screen 'Not Found' page for all sections except the 'More Features' page.
This is because the 'More Features' page is the only one where we don’t pass a featureName. App/src/pages/workspace/WorkspaceMoreFeaturesPage.tsx Lines 397 to 400 in ba9af8f
What changes do you think we should make in order to solve the problem?We should also display a full-screen 'Not Found' page for the features page. There are several ways to achieve this, one of which is by passing props. For features page we can pass a new prop App/src/pages/workspace/WorkspaceMoreFeaturesPage.tsx Lines 397 to 400 in ba9af8f
And then we can use it here
This will make it consistent with other pages. Screen.Recording.2024-10-03.at.11.28.20.PM.movWe can fix this in other parts of app too where we have this problem What alternative solutions did you explore? (Optional) |
PROPOSAL UPDATED
|
I am not sure if this bug worth fixing, i think its a minor issue |
📣 @hayes102! 📣
|
Job added to Upwork: https://www.upwork.com/jobs/~021843399831747959638 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
@aimane-chnaif - when you get a chance, can you please review these proposals? Thanks! |
Thanks for the proposals. @Nodebrute the root cause is not correct.
We don't display full-screen 'Not found' on categories page when this feature is enabled. Full-screen is only when the feature is disabled. |
Note that the bug is not limited to More Features page but also happens on other workspace settings pages like Categories. I think just removing back button for wide screen (while showing policy side bar) is fine. |
@aimane-chnaif, updating in a hour. |
@aimane-chnaif, could you please share more details about the regressions you are talking about? Monosnap.screencast.2024-10-08.15-27-53.mp4 |
@Krishna2323 please share your branch |
@Krishna2323's proposal (main solution) looks good to me. |
Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR will be up within 24 hours. |
@aimane-chnaif, PR ready for review ^ |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v9.0.44-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): expensify416+da2@gmail.com
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
There shouldn't be back button on not found page for wide screen. It should be available only for small screen (mobile)
Actual Result:
Back button is available for wide screen. That makes it a redundant back button for workspace page.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6623300_1727966545832.Screen_Recording_2024-10-03_at_5.34.51_in_the_afternoon.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @aimane-chnaifThe text was updated successfully, but these errors were encountered: