Disabled clickable & hoverable state in the banner if there's no clickable element in it. #13576

priyeshshah11 · 2022-12-14T01:14:08Z

Details

Fixed Issues

$ #13452
PROPOSAL: #13452 (comment)

Tests

Go to any archived room.
Hover over the “This chat room has been archived.” text.
Verify that the banner colour doesn't change and there is no change in the mouse cursor too.
Go to a conceirge report with an account manager.
Hover over "Chat with your account manager here".
Verify that the banner colour changes and the mouse cursor changes to a click pointer.

Verify that no errors appear in the JS console

Offline tests

Go to any archived room.
Hover over the “This chat room has been archived.” text.
Verify that the banner colour doesn't change and there is no change in the mouse cursor too.
Go to a conceirge report with an account manager.
Hover over "Chat with your account manager here".
Verify that the banner colour changes and the mouse cursor changes to a click pointer.

Verify that no errors appear in the JS console

QA Steps

Go to any archived room.
Hover over the “This chat room has been archived.” text.
Verify that the banner colour doesn't change and there is no change in the mouse cursor too.
Go to a conceirge report with an account manager.
Hover over "Chat with your account manager here".
Verify that the banner colour changes and the mouse cursor changes to a click pointer.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

banner-web.mov

Mobile Web - Chrome

banner-mweb-chrome.mov

Mobile Web - Safari

banner-mweb-safari.mov

Desktop

banner-desktop.mov

iOS

banner-ios1.mov

Android

banner-android.mov

…kable element in it

melvin-bot · 2022-12-14T01:14:13Z

@puneetlath @Santhosh-Sellavel One of you needs to 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]

priyeshshah11 · 2022-12-14T01:17:23Z

The diff looks huge due to indentation but the key changes are on lines (new) 48, 49, 57, 58, 65, 75

src/components/Banner.js

Santhosh-Sellavel

Okay, it's too many changes, Having second thoughts about, Lot of unnecessary changes now. Maybe let's just introduce a prop, I think that's simpler any thoughts @puneetlath?

priyeshshah11 · 2022-12-14T23:41:59Z

Okay, it's too many changes, Having second thoughts about, Lot of unnecessary changes now. Maybe let's just introduce a prop, I think that's simpler any thoughts @puneetlath?

@Santhosh-Sellavel Did you check it with "Hide whitespace" tick marked?

priyeshshah11 · 2022-12-15T01:40:50Z

Maybe let's just introduce a prop, I think that's simpler any thoughts @puneetlath?

@Santhosh-Sellavel I don't think having too many props is ideal when we can already deduce things from existing props.
For e.g. shouldShowCloseButton isn't necessary as you would only want to show the close button when onClose prop is passed

But happy to hear others' thoughts @puneetlath

puneetlath · 2022-12-15T14:08:05Z

Oh wow, I never knew about the "hide whitespace" setting option. That's cool. Looking at it after selecting that I feel comfortable with the changes as long as it tests well.

priyeshshah11 · 2022-12-15T14:11:54Z

Oh wow, I never knew about the "hide whitespace" setting option. That's cool. Looking at it after selecting that I feel comfortable with the changes as long as it tests well.

Ohh nice, now you know! 😁 In regards to testing, it does work well. All platform's test videos attached.

bondydaa

👍 overall, I initially was concerned about this and thought maybe we should have just solved it with a different style applied but after trying to come up with the how of that idea I realize it's not that straight forward so am good with this solution.

I'm pretty new to the review flow, should I copy some checklist?

puneetlath · 2022-12-15T19:21:02Z

@bondydaa at least one reviewer needs to complete the full checklist. Typically the C+ (in this case @Santhosh-Sellavel) will take care of that.

Santhosh-Sellavel · 2022-12-15T23:46:21Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

2022-12-15_18-40-14.mp4

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2022-12-16.at.6.06.26.AM.mov

iOS & Android

Screen.Recording.2022-12-16.at.6.10.03.AM.mov

Santhosh-Sellavel · 2022-12-15T23:48:53Z

@priyeshshah11

Can you add recording for this or explain how to do this, So I can test this quickly!
Go to a conceirge report with an account manager.
Hover over "Chat with your account manager here".

priyeshshah11 · 2022-12-16T00:01:15Z

@priyeshshah11

Can you add recording for this or explain how to do this, So I can test this quickly! Go to a conceirge report with an account manager. Hover over "Chat with your account manager here".

@Santhosh-Sellavel These are the conditions when it's shown but I don't know exactly when do we have an accountManagerReportID in a report. I tested the component itself in isolation by commenting those conditions.

{this.props.accountManagerReportID && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && (
    <Banner
        containerStyles={[styles.mh4, styles.mt4, styles.p4, styles.bgDark]}
        textStyles={[styles.colorReversed]}
        text={this.props.translate('reportActionsView.chatWithAccountManager')}
        onClose={this.dismissBanner}
        onPress={this.chatWithAccountManager}
        shouldShowCloseButton
    />

)}

Screen.Recording.2022-12-16.at.11.00.04.AM.mov

Santhosh-Sellavel · 2022-12-16T00:04:23Z

@priyeshshah11 Seems there is no hover effect there?

priyeshshah11 · 2022-12-16T00:09:25Z

@priyeshshah11 Seems there is no hover effect there?

yes and that's expected as that's how that component is without my changes too, because of the styles.bgDark being passed in containerStyles which overrides the hover styles. Do we want to change that behaviour?

Santhosh-Sellavel · 2022-12-16T00:18:20Z

@Expensify/design is this banner working as expected?

bondydaa · 2022-12-16T00:29:49Z

I think that's right based on my local environment right now that is pretty up to date with main (pulled earlier today but not since)

2022-12-15_17-28-46.mp4

Santhosh-Sellavel · 2022-12-16T00:32:41Z

@bondydaa I'm not sure how to get the account manager, So I'll add screens for others since you covered the account manager case!

bondydaa · 2022-12-16T00:37:27Z

sounds good, the account manager stuff is tied to an NVP on the backend which we'd probably have to manually insert it into the DB for it to show up on one of your accounts.

Let me pull your code locally and upload another screenshot but I think it'll end up looking the same as the one you got

bondydaa · 2022-12-16T00:46:26Z

steps I did to pull your branch

git remote add test git@github.com:priyeshshah11/App.git
git fetch
git checkout test/fix-13452

Ran the app and it looks the same as far as I can tell

2022-12-15_17-45-42.mp4

shawnborton · 2022-12-16T00:51:42Z

Oh interesting, so there is no clickable link in that banner?

One thing to note here - we need to update the border radius of this banner to use componentBorderRadius and not buttonBorderRadius

Santhosh-Sellavel · 2022-12-16T00:52:44Z

Oh interesting, so there is no clickable link in that banner?

We have for account manager

shawnborton · 2022-12-16T00:55:26Z

Got it. So yeah, if something is clickable, it makes sense to have a hover state for the cursor.

Santhosh-Sellavel · 2022-12-16T01:00:21Z

Got it. So yeah, if something is clickable, it makes sense to have a hover state for the cursor.

The cursor works fine, but do we need a hover background here?

bondydaa · 2022-12-16T01:06:37Z

At the very least I don't think this adds any regressions with the account manager banner with these changes.

My feelings would be if you want to add the extra go for it but we can also spin up a new issue to add those states as well, thoughts @shawnborton ?

shawnborton · 2022-12-16T01:08:36Z

I think just the cursor is fine, no need for BG changes?

Santhosh-Sellavel

@bondydaa

Checklist: #13576 (comment)

Didn't add the web platforms screens, I am unable to add them due to login issues on the web. But this should work as I tested it on the Desktop. So Approving this one.

Santhosh-Sellavel · 2022-12-16T01:15:46Z

@priyeshshah11
Can you check whether check list is up to date with the latest PR Template here?

priyeshshah11 · 2022-12-16T01:25:37Z

@priyeshshah11 Can you check whether check list is up to date with the latest PR Template here?

@Santhosh-Sellavel done now, thanks!

bondydaa · 2022-12-16T01:41:46Z

Didn't add the web platforms screens

Oh I guess I could probably test that too 🤦 followed the same steps to checkout the fork

✅
https://user-images.githubusercontent.com/4073354/208002454-1ae59566-5b6c-4c0e-8742-196550ddcf8f.mp4

bondydaa · 2022-12-16T01:47:45Z

I can't seem to get mobile web to work for me either

Santhosh-Sellavel · 2022-12-16T02:27:04Z

I requested access here, https://expensify.slack.com/archives/C01GTK53T8Q/p1671150365051029?thread_ts=1670957921.425999&cid=C01GTK53T8Q. But still didn't get access, can you help here @bondydaa ?

bondydaa · 2022-12-16T17:23:24Z

unfortunately i don't have access to our cloudflare so can't help expedite that.

@puneetlath @arosiclair do either of you have working local environments and could follow the steps here #13576 (comment) to check this code out locally and confirm mobile web for us?

To create an archived room, create a new workspace then close it and those will then show as "archived"

ultimately I think it's pretty low risk if we can't confirm before merging - once this is on staging we'll be able to confirm for sure right?

Santhosh-Sellavel · 2022-12-16T21:44:58Z

@bondydaa Let's merge this, I'm it won't break anything also we have a recording from PR Description which confirms that. Also it doesn't have any effect on mWeb as we don't hover there.

OSBotify · 2022-12-16T22:13:33Z

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

github-actions · 2022-12-16T22:30:14Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	795.082 ms → 834.236 ms (+39.154 ms, +4.9%)
runJsBundle	191.875 ms → 201.750 ms (+9.875 ms, +5.1%)
nativeLaunch	9.613 ms → 9.774 ms (+0.161 ms, +1.7%)
regularAppStart	0.016 ms → 0.014 ms (-0.002 ms, -11.7%)

Show details

Name	Duration
TTI	Baseline Mean: 795.082 ms Stdev: 36.208 ms (4.6%) Runs: 738.3287559999153 747.6048459997401 749.775117999874 750.0549269998446 750.5929439999163 757.1761569995433 758.4309870004654 758.5508239995688 763.5236870003864 774.7814589999616 775.300741000101 776.1681000003591 781.1812340002507 782.490288999863 784.7726280000061 794.5457300003618 794.7452699998394 804.5568000003695 809.4362949999049 813.7276060003787 815.0270919995382 816.6193340001628 817.8333679996431 820.1790589997545 820.8053500000387 827.6685819998384 828.9641589997336 837.9945059996098 861.1618980001658 861.6314070001245 873.926741999574 Current Mean: 834.236 ms Stdev: 28.859 ms (3.5%) Runs: 775.1366910003126 776.6452249996364 786.5239580003545 799.5057760002092 802.6278050001711 810.9229499995708 812.9719280004501 813.702237999998 819.0761050004512 823.200637999922 828.4464579997584 829.7988579999655 830.999552000314 831.3449619999155 835.7797400001436 837.5787690002471 838.7357430001721 839.7887199996039 840.2822329998016 846.4302589995787 848.1235589999706 850.9905470004305 853.9934289995581 855.8742570001632 860.3111159997061 861.5844419999048 865.7011970002204 874.2423029998317 874.4628280000761 902.3125449996442
runJsBundle	Baseline Mean: 191.875 ms Stdev: 24.740 ms (12.9%) Runs: 155 158 162 163 163 166 167 169 172 173 177 178 182 182 185 186 186 188 193 198 206 209 209 209 214 216 220 221 224 226 230 253 Current Mean: 201.750 ms Stdev: 24.560 ms (12.2%) Runs: 160 167 168 170 171 179 183 184 185 186 188 190 196 196 199 203 205 205 206 207 207 209 210 216 216 217 218 219 226 249 259 262
nativeLaunch	Baseline Mean: 9.613 ms Stdev: 1.430 ms (14.9%) Runs: 7 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 11 11 11 11 11 12 12 13 Current Mean: 9.774 ms Stdev: 1.913 ms (19.6%) Runs: 8 8 8 8 8 8 8 8 8 8 8 8 8 9 9 10 10 10 10 10 10 10 11 11 11 12 12 13 13 14 14
regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.001 ms (6.1%) Runs: 0.014527000486850739 0.014566999860107899 0.01464799977838993 0.014852000400424004 0.014933000318706036 0.015096000395715237 0.015217999927699566 0.01534000039100647 0.015381000004708767 0.015461999922990799 0.015461999922990799 0.015502999536693096 0.015747000463306904 0.015786999836564064 0.015949999913573265 0.015951000154018402 0.016193999908864498 0.016195000149309635 0.016195000149309635 0.0163569999858737 0.016439000144600868 0.01660200022161007 0.0166830001398921 0.0166830001398921 0.0167239997535944 0.0168869998306036 0.0172520000487566 0.017253000289201736 0.017659000121057034 0.01887999940663576 Current Mean: 0.014 ms Stdev: 0.001 ms (4.6%) Runs: 0.012614000588655472 0.013346999883651733 0.013467999175190926 0.013549000024795532 0.01358999963849783 0.013630999252200127 0.013631000183522701 0.013712000101804733 0.01371300034224987 0.01371300034224987 0.0138349998742342 0.01387499924749136 0.013915999792516232 0.0139979999512434 0.0139979999512434 0.0139979999512434 0.014038000255823135 0.014038000255823135 0.014078999869525433 0.014282000251114368 0.014403999783098698 0.01448499970138073 0.014525999315083027 0.014526000246405602 0.014933000318706036 0.015054999850690365 0.015135999768972397 0.01521800085902214 0.015381000004708767 0.015420999377965927

priyeshshah11 · 2022-12-17T02:29:52Z

Thanks team! 😄 @Santhosh-Sellavel @bondydaa @puneetlath @arosiclair

OSBotify · 2022-12-20T06:46:55Z

🚀 Deployed to staging by @bondydaa in version: 1.2.42-0 🚀

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

OSBotify · 2022-12-21T22:57:47Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀

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

priyeshshah11 added 2 commits December 14, 2022 11:27

disabled clickable & hoverable state in the banner if there's no clic…

755e365

…kable element in it

refactored conditions to follow DRY

38ddc66

priyeshshah11 requested a review from a team as a code owner December 14, 2022 01:14

melvin-bot bot requested review from puneetlath and Santhosh-Sellavel and removed request for a team December 14, 2022 01:14

arosiclair reviewed Dec 14, 2022

View reviewed changes

src/components/Banner.js Show resolved Hide resolved

Santhosh-Sellavel reviewed Dec 14, 2022

View reviewed changes

bondydaa approved these changes Dec 15, 2022

View reviewed changes

puneetlath approved these changes Dec 15, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Dec 16, 2022

View reviewed changes

bondydaa merged commit c71c13d into Expensify:main Dec 16, 2022

OSBotify mentioned this pull request Dec 20, 2022

Deploy Checklist: New Expensify 2022-12-20 #13723

Closed

34 tasks

priyeshshah11 mentioned this pull request Dec 29, 2022

[$1000] The cursor is displayed as pointer on the archived room banner text #13452

Closed

Disabled clickable & hoverable state in the banner if there's no clickable element in it. #13576

Disabled clickable & hoverable state in the banner if there's no clickable element in it. #13576

Conversation

priyeshshah11 commented Dec 14, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 14, 2022

priyeshshah11 commented Dec 14, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

priyeshshah11 commented Dec 14, 2022

priyeshshah11 commented Dec 15, 2022

puneetlath commented Dec 15, 2022

priyeshshah11 commented Dec 15, 2022

bondydaa left a comment

Choose a reason for hiding this comment

puneetlath commented Dec 15, 2022

Santhosh-Sellavel commented Dec 15, 2022 • edited by bondydaa Loading

Reviewer Checklist

Screenshots/Videos

Santhosh-Sellavel commented Dec 15, 2022

priyeshshah11 commented Dec 16, 2022 • edited Loading

Santhosh-Sellavel commented Dec 16, 2022

priyeshshah11 commented Dec 16, 2022

Santhosh-Sellavel commented Dec 16, 2022 • edited Loading

bondydaa commented Dec 16, 2022

Santhosh-Sellavel commented Dec 16, 2022

bondydaa commented Dec 16, 2022

bondydaa commented Dec 16, 2022

shawnborton commented Dec 16, 2022

Santhosh-Sellavel commented Dec 16, 2022

shawnborton commented Dec 16, 2022

Santhosh-Sellavel commented Dec 16, 2022 • edited Loading

bondydaa commented Dec 16, 2022 • edited Loading

shawnborton commented Dec 16, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Dec 16, 2022

priyeshshah11 commented Dec 16, 2022

bondydaa commented Dec 16, 2022

bondydaa commented Dec 16, 2022

Santhosh-Sellavel commented Dec 16, 2022

bondydaa commented Dec 16, 2022

Santhosh-Sellavel commented Dec 16, 2022

OSBotify commented Dec 16, 2022

github-actions bot commented Dec 16, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

priyeshshah11 commented Dec 17, 2022

OSBotify commented Dec 20, 2022

OSBotify commented Dec 21, 2022

priyeshshah11 commented Dec 14, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 15, 2022 •

edited by bondydaa

Loading

priyeshshah11 commented Dec 16, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 16, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 16, 2022 •

edited

Loading

bondydaa commented Dec 16, 2022 •

edited

Loading