feat: Add temporary focus to enabled option in workspace initial page #38546

allroundexperts · 2024-03-18T23:09:53Z

Details

Adds temporary focus to the workspace feature that was enabled on narrow screens.

Fixed Issues

$ #38372
PROPOSAL: N/A

Tests

For narrow screen layouts only:

Open app and go to any collect workspace
Tap more features
Enable any option

Verify that the row of the enabled option is highlighted when we take the user back to the LHN.

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

For narrow screen layouts only:

Open app and go to any collect workspace
Tap more features
Enable any option

Verify that the row of the enabled option is highlighted when we take the user back to the LHN.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-03-19.at.4.05.23.AM.mov

Android: mWeb Chrome

Screen.Recording.2024-03-19.at.4.04.50.AM.mov

iOS: Native

Screen.Recording.2024-03-19.at.4.03.14.AM.mov

iOS: mWeb Safari

Screen.Recording.2024-03-19.at.4.02.46.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-03-19.at.3.32.13.AM.mov

MacOS: Desktop

Screen.Recording.2024-03-19.at.3.34.48.AM.mov

melvin-bot · 2024-03-18T23:24:56Z

@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

allroundexperts · 2024-03-18T23:25:39Z

I think @rojiphil is supposed to review this cc @mountiny

mountiny · 2024-03-18T23:36:22Z

@allroundexperts nice! any chance it could fade away?

cc @Expensify/engineering team for 👁️ maybe we want to update the time

mountiny · 2024-03-18T23:37:13Z

I believe we want the effect to be more of a "highlight", I will leave that to the design team though

allroundexperts · 2024-03-18T23:38:29Z

@allroundexperts nice! any chance it could fade away?

This is similar to how its being done in #38036. We don't have the fade effect there or do we?

allroundexperts · 2024-03-18T23:39:51Z

Just for the reference of the design team, the focus time is 3s right now.

thienlnam · 2024-03-18T23:43:46Z

I think you tagged the wrong team here heh @mountiny

src/pages/workspace/WorkspaceInitialPage.tsx

src/components/MenuItem.tsx

allroundexperts · 2024-03-19T08:20:39Z

@rojiphil This is not how it should work. I'll check and get back!

allroundexperts · 2024-03-19T10:13:56Z

@allroundexperts @mountiny Please find the test video for the above implementation. Does this look fine to you?

38546-highlight-fix.mp4

@rojiphil Is this running on my branch? I can't seem to reproduce this behaviour.

allroundexperts · 2024-03-19T10:14:24Z

Since highlight was the original expectation, please check if the following implementation would meet the requirement of highlight implementation

Isn't highlight the same as temporary focus?

allroundexperts · 2024-03-19T10:15:07Z

The current changes are for the categories feature. Further, this has the advantage of remembering the previous active route and highlighting the same.

Not sure what you're talking about here.

mountiny · 2024-03-19T11:07:58Z

Oh damn sorry for the wrong tag here team, typo + autosuggestion

It was meant to be @Expensify/design

rojiphil · 2024-03-19T11:21:44Z

Isn't highlight the same as temporary focus?

Hmm... It seemed to me that the expectation was to keep the highlight on and not have a temporary focus. If the expectation is to have a temporary focus then all what I said can be ignored.

shawnborton · 2024-03-19T13:05:40Z

So just to make sure we're all on the same page here:

On mobile, when you enable a feature from the More features page, you navigate back to the workspace editor navigation menu
When you get back to the workspace navigation menu, we want to show a visual highlight of the feature we just enabled

I think that's all we're trying to accomplish here, right?

What I think I would recommend is that we highlight the newly enabled feature by:

giving it the :active background color, which is the same as our border color or product400
we also make the icon of the highlighted nav item use our success/green400 color

I think the highlight should stay for like 1.5-2.5 seconds, and when the highlight is over, we should fade it out with a slight transition instead of instant.

Curious if that is what the rest of the @Expensify/design is thinking too.

dannymcclain · 2024-03-19T13:53:25Z

@shawnborton yeah that sounds good to me. 👍

I also wonder if we could add a slight delay to the transition from the more features page to the navigation menu? Right now it happens so fast you can't even see the toggle go all the way to the on state. I think a tiny amount of delay there would really help it feel not so jarring.

shawnborton · 2024-03-19T13:57:57Z

Oh yeah, I love that idea!

src/components/MenuItem.tsx

shawnborton · 2024-04-04T07:11:22Z

though I will say that the mobile animation seems to be cut off at the start due to the navigation up the hierarchy.

Yup, this stood out to me too.

rojiphil · 2024-04-04T07:12:09Z

@allroundexperts Looks like we introduced regression here. Can you also please check on this?

38372-pr-branch.mp4

rojiphil · 2024-04-04T08:23:52Z

I just checked on all the platforms. To me, the feature works like a charm on all. Just that while testing with a small screen device scenario on MacOS:safari platform, I noticed that the highlight animation is quite quick (as can be seen in the test video). But, maybe we can ignore this as mobile platforms seem to work well.

Screenshots/Videos

Android: Native

38372-android-native.mp4

Android: mWeb Chrome

38372-mweb-chrome.mp4

iOS: Native

38372-ios-native.mp4

iOS: mWeb Safari

38372-mweb-safari.mp4

MacOS: Safari

38372-web-safari.mp4

MacOS: Desktop

38372-desktop.mp4

rojiphil · 2024-04-04T08:25:54Z

though I will say that the mobile animation seems to be cut off at the start due to the navigation up the hierarchy

@dubielzyk-expensify @shawnborton Just to be on the same page, did you mean a similar observation (as mentioned here) as on testing a MacOS:safari platform with small screen device scenario? Or did you mean something else?

shawnborton · 2024-04-04T09:13:12Z

I think it's a similar comment, yes. Basically the highlight animation seems to start slightly before the page transition (goBack) happens.

mountiny

Its shaping up well now, no major comments to the code, I think we are at the point we should get this to staging and feel it out

mountiny · 2024-04-04T11:56:21Z

Except this regression obviously

allroundexperts · 2024-04-04T11:58:07Z

Regression is fixed. @rojiphil Please verify.

allroundexperts · 2024-04-04T12:00:59Z

Also, @mountiny @luacmartins, just to be sure that we're on the same page, with this useEffect approach, the items might get highlighted if a feature is enabled on another device. I can't see any good way to avoid this unless we make use of some kind of state variable that tells that the user came back from the enable feature page explicitly.

rojiphil

Looks like we introduced few more issues. But I think we are very close. Please check.

rojiphil · 2024-04-04T13:04:28Z

src/components/HighlightableMenuItem.tsx

@@ -0,0 +1,36 @@
+import type {ForwardedRef} from 'react';
+import {forwardRef} from 'react';


Looks like we missed adding React here.

It's weird that there is no lint / type error for this. I've added it anyways.

rojiphil · 2024-04-04T13:07:35Z

src/components/HighlightableMenuItem.tsx

+        <MenuItem
+            // eslint-disable-next-line react/jsx-props-no-spreading
+            {...restOfProps}
+            wrapperStyle={animatedHighlightStyle}


Did you mean outerWrapperStyle={animatedHighlightStyle}?

Yep, my bad. Fixed.

src/components/HighlightableMenuItem.tsx

luacmartins

Looking good

src/hooks/useAnimatedHighlightStyle/config.ts

rojiphil · 2024-04-04T13:23:43Z

the items might get highlighted if a feature is enabled on another device.

Yea. Thanks for bringing this up. I had captured this in my iOS:Native platform testing here. Please use the test video for reference if needed. But, it looked a nice little feature to me.

rojiphil

Awesome. Tests well on all platforms. LGTM.

luacmartins

LGTM

mountiny

Thank you!

OSBotify · 2024-04-04T16:41:36Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-04-08T11:08:14Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.61-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-04-08T13:04:52Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.61-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-04-10T17:22:29Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.61-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

add temp focus

8c09ebe

allroundexperts changed the title ~~feat: Add temp focus to workspace initial page~~ feat: Add temporary focus to enabled option in workspace initial page Mar 18, 2024

allroundexperts marked this pull request as ready for review March 18, 2024 23:24

allroundexperts requested a review from a team as a code owner March 18, 2024 23:24

melvin-bot bot requested review from rushatgabhane and removed request for a team March 18, 2024 23:24

allroundexperts mentioned this pull request Mar 18, 2024

[Awaiting Payment 30th April] [Wave Collect] [More Features] [$500] Feature enabled from more features are not highlighted when user taken back to the LHN #38372

Closed

6 tasks

mountiny assigned allroundexperts Mar 18, 2024

mountiny requested a review from rojiphil March 18, 2024 23:35

fedirjh reviewed Mar 19, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

luacmartins self-requested a review March 19, 2024 18:43

luacmartins reviewed Mar 19, 2024

View reviewed changes

src/components/MenuItem.tsx Outdated Show resolved Hide resolved

Merge branch 'main' into fix-38372

904ee16

mountiny previously approved these changes Apr 4, 2024

View reviewed changes

fix regression

942d665

allroundexperts dismissed mountiny’s stale review via 942d665 April 4, 2024 11:57

lint

c5e509b

rojiphil suggested changes Apr 4, 2024

View reviewed changes

luacmartins reviewed Apr 4, 2024

View reviewed changes

src/hooks/useAnimatedHighlightStyle/config.ts Show resolved Hide resolved

handle comments

64c2a45

rojiphil approved these changes Apr 4, 2024

View reviewed changes

melvin-bot bot requested a review from mountiny April 4, 2024 14:14

luacmartins approved these changes Apr 4, 2024

View reviewed changes

mountiny approved these changes Apr 4, 2024

View reviewed changes

luacmartins merged commit de09508 into Expensify:main Apr 4, 2024
17 of 21 checks passed

github-actions bot mentioned this pull request Apr 8, 2024

Deploy Checklist: New Expensify 2024-04-08 #39807

Closed

kbecciv mentioned this pull request Apr 8, 2024

[HOLD for payment 2024-06-06] [HOLD for payment 2024-05-06] Web - Features - Feature row in left side bar appears with delay after the page opens on the right #39841

Closed

6 tasks

This was referenced Apr 12, 2024

Deploy Checklist: New Expensify 2024-04-08 #39807 Derevyaa/Expensify-App#1

Closed

Deploy Checklist: New Expensify 2024-04-09 #39808 Derevyaa/Expensify-App#2

Closed

		@@ -0,0 +1,36 @@
		import type {ForwardedRef} from 'react';
		import {forwardRef} from 'react';

feat: Add temporary focus to enabled option in workspace initial page #38546

feat: Add temporary focus to enabled option in workspace initial page #38546

Conversation

allroundexperts commented Mar 18, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Mar 18, 2024

allroundexperts commented Mar 18, 2024

mountiny commented Mar 18, 2024

mountiny commented Mar 18, 2024

allroundexperts commented Mar 18, 2024 • edited Loading

allroundexperts commented Mar 18, 2024

thienlnam commented Mar 18, 2024

This comment was marked as off-topic.

This comment was marked as outdated.

allroundexperts commented Mar 19, 2024 • edited Loading

allroundexperts commented Mar 19, 2024

allroundexperts commented Mar 19, 2024

allroundexperts commented Mar 19, 2024

mountiny commented Mar 19, 2024

rojiphil commented Mar 19, 2024

shawnborton commented Mar 19, 2024

dannymcclain commented Mar 19, 2024

shawnborton commented Mar 19, 2024

shawnborton commented Apr 4, 2024

rojiphil commented Apr 4, 2024

rojiphil commented Apr 4, 2024

Screenshots/Videos

rojiphil commented Apr 4, 2024

shawnborton commented Apr 4, 2024

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Apr 4, 2024

allroundexperts commented Apr 4, 2024

allroundexperts commented Apr 4, 2024

rojiphil left a comment

Choose a reason for hiding this comment

rojiphil Apr 4, 2024

Choose a reason for hiding this comment

allroundexperts Apr 4, 2024

Choose a reason for hiding this comment

rojiphil Apr 4, 2024

Choose a reason for hiding this comment

allroundexperts Apr 4, 2024

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

rojiphil commented Apr 4, 2024

rojiphil left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 4, 2024

OSBotify commented Apr 8, 2024

OSBotify commented Apr 8, 2024

OSBotify commented Apr 10, 2024

allroundexperts commented Mar 18, 2024 •

edited

Loading

allroundexperts commented Mar 18, 2024 •

edited

Loading

allroundexperts commented Mar 19, 2024 •

edited

Loading