Open Krishna2323 opened 1 month ago
@akinwale 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]
@shawnborton https://github.com/Expensify/App/issues/37782#issuecomment-2009382463, do you have something to suggest here? it would be great if we clear things before I start implementing it on other pages since we need to handle multiple pages, eg: tags, categories, taxes, distance, etc etc.
https://github.com/Expensify/App/assets/85894871/058bb4ad-279d-40d7-a9bf-9b8ee714bc5d
Thanks for the video! cc @Expensify/design for visibility
One thing that would be nice, but would be more difficult - it would be cool if the entire page was able to scroll below the header area, but as soon as the table column header row (thead)
was about to scroll out of view, it would stay fixed to the top. This way the description can scroll away so you get a little more viewport height to use. Does that make sense, and is that doable?
@shawnborton, my apologies for the delay. I encountered some difficulties over the past few days while attempting to implement your suggestions. However, I believe I've finally managed to grasp it. Could you please confirm if this is what you meant?
https://github.com/Expensify/App/assets/85894871/d18f64a3-0a8c-4961-8a8e-daeb45123c77
Exactly! Though it looks like the thead has an incorrect BG color?
@shawnborton, Yep, I just added white as bg for testing because it was transparent and hard to see in demo video. Will update that. Thanks.
One more thing I would suggest is that we should scroll to the top when the user selects all by using the checkbox in the thead
. This way, they can see the dropdown button to delete/disable/enable categories/tags/etc.
@shawnborton, could you please tell me what background to use for the sticky header? The application's background is set to darkMode: rgba(6,27,9,0.92)
and lightMode: rgba(252, 251, 249, 0.92)
. I applied the same background to the header, but the options are still visible when the header overlaps them because of the opacity set to.92
.
https://github.com/Expensify/App/assets/85894871/a89f9b4c-906f-497a-9603-919a8e52fd99
Whoa - no idea why the opacity is reduced. Can we just remove that and make it so that it uses a 100% opacity background?
yep, we can create a new variable for that to use the same bg color with full opacity, will share screenshots after making changes.
Do we need a new variable? It should just be the same as the appBG right?
Sorry for the confusion, I was lost in elements tab. We are using 252, 251, 249
with full opacity for the app. You can ignore the issue I mentioned above.
Code changes are completed, will do final testing and add recordings for review.
Facing a weird issue :(, the LHS is not displayed when visiting any policy settings page. Will try again later.
https://github.com/Expensify/App/assets/85894871/45af4c52-5c83-4fc3-9d81-b27037d294ca
Looks like we have some conflicts.
@akinwale, you can start reviewing this, I'm facing some issues while building the app for iOS. I'm working on fixing the build issue and will add screenshots for both Android and iOS shortly. All other recordings have been added.
@Krishna2323 Please fix merge conflicts.
@akinwale, conflicts resolved.
@akinwale, please hold off on reviewing this. While recording the video for Android, I noticed that the thead
doesn't stick to the top in the Android native app, whereas it does in the iOS native app. Please wait until I find a solution for this, I'm currently working on it.
I still haven't found any solution for Android native development :( However, I did notice one weird behavior, when I initially visit any policy page with a selection list, the table header doesn't stick to the top. But when I navigate to another page from the policy page and then return, the table header starts working correctly and sticks to the top.
I'm planning to test this on a physical Android device, maybe it will work there. @akinwale, do you have any suggestions on how to set up running the app on a physical device? I haven't tried it yet.
https://github.com/Expensify/App/assets/85894871/304e803c-2520-4185-85ee-ef2911f2fce8
@akinwale @shawnborton, I spent a lot of time finding a solution for android native thead stick issue but hasn't found any solution yet. Should I try workaround. I found few: medium, stackoverflow
Curious what @akinwale thinks there
@Krishna2323 I think it would make sense as long as it's implemented as a platform-specific component for Android-native only.
@akinwale @shawnborton, I've tried implementing both solutions, but unfortunately, they aren't working as expected. There's a noticeable lag while scrolling.
https://github.com/Expensify/App/assets/85894871/2330833e-63c9-4dcb-b48e-52f5575a9cc8
Hmm I am out of ideas then. At this point, maybe we should just make this part of the view fixed:
And everything else can scroll under it. Thoughts?
@shawnborton, IMO making the buttons stick to the top, instead of select all rows thead
, makes more sense for a few reasons:
Yeah, that makes sense to me and I think it's a good compromise.
@akinwale, code is updated according to the discussion above, you can start the review. Recordings will be updated in few moments.
@akinwale recordings are updated. @shawnborton can you pls verify the behaviour in the recording below.
https://github.com/Expensify/App/assets/85894871/de69a238-d56e-45b7-a21e-9ab7b48dfc5b
That feels pretty good to me. Thoughts @Expensify/design?
Yeah I think that feels pretty good.
LGTM too!
@akinwale, bump for review.
Reviewing today.
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
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)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
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
is working as expected in all cases)Design
label and/or tagged @Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test
steps.@flodnv, friendly bump to merge this.
Details
Fixed Issues
$ https://github.com/Expensify/App/issues/37782 PROPOSAL: https://github.com/Expensify/App/issues/37782#issuecomment-1984112170
Tests
For devices with width greater than
800px
Members
,Distance rates
,Categories
,Tags
andTaxes
page.For devices with width less than
800px
Members
,Distance rates
,Categories
,Tags
andTaxes
page.thead
scrolls and buttons are fixed.Offline tests
For devices with width greater than
800px
Members
,Distance rates
,Categories
,Tags
andTaxes
page.For devices with width less than
800px
Members
,Distance rates
,Categories
,Tags
andTaxes
page.thead
scrolls and buttons are fixed.QA Steps
For devices with width greater than
800px
Members
,Distance rates
,Categories
,Tags
andTaxes
page.For devices with width less than
800px
Members
,Distance rates
,Categories
,Tags
andTaxes
page.thead
scrolls and buttons are fixed.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
https://github.com/Expensify/App/assets/85894871/de69a238-d56e-45b7-a21e-9ab7b48dfc5bAndroid: mWeb Chrome
https://github.com/Expensify/App/assets/85894871/dc281d4c-3197-4aec-a49b-9866b6ff26d9iOS: Native
https://github.com/Expensify/App/assets/85894871/b2ec120a-dac9-42e4-aac1-771a2fa5eb8eiOS: mWeb Safari
https://github.com/Expensify/App/assets/85894871/633268c7-30ca-4a43-8ca8-bc80b6e7b6eeMacOS: Chrome / Safari
https://github.com/Expensify/App/assets/85894871/568a4a90-201e-4be1-8e42-a24c01761f0cMacOS: Desktop
https://github.com/Expensify/App/assets/85894871/630d2bb7-4495-40ef-9b37-83a49ee2f4ff