-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
New preferences page layout #21852
New preferences page layout #21852
Conversation
This PR is branched off of #20663, so the diff will be smaller once that's merged. |
d30cca4
to
090ae41
Compare
@stitesExpensify 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 is going to review this |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-07-17.at.12.44.07.AM.movMobile Web - SafariScreen.Recording.2023-07-17.at.1.02.44.AM.movDesktopScreen.Recording.2023-07-17.at.12.50.36.AM.moviOSScreen.Recording.2023-07-17.at.1.02.09.AM.movAndroidScreen.Recording.2023-07-17.at.1.03.20.AM.mov |
container: this.animationDOMNode, | ||
animationData: props.source, | ||
- renderer: 'svg', | ||
+ renderer: 'canvas', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this seems to yield better framerates and also fixes a display but on iOS Safari with different animation (not yet included in the app)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roryabraham Can you post the screenshots? I saw some screenshots in the linked issue but since this is a partial implementation, it would be good to have the screenshots that this PR is implementing. |
@allroundexperts refresh the page maybe? GitHub was having issues but I posted all the screenshots before putting this in for review. I can still see them in the issue description |
@allroundexperts all fixed now |
export default function withEnvironment(WrappedComponent) { | ||
const WithEnvironment = forwardRef((props, ref) => { | ||
const {environment, environmentURL} = useContext(EnvironmentContext); | ||
return ( | ||
<WrappedComponent | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
ref={props.forwardedRef} | ||
ref={ref} | ||
environment={environment} | ||
environmentURL={environmentURL} | ||
/> | ||
); | ||
} | ||
}); | ||
|
||
WithEnvironment.displayName = `withEnvironment(${getComponentDisplayName(WrappedComponent)})`; | ||
WithEnvironment.propTypes = { | ||
forwardedRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(React.Component)})]), | ||
}; | ||
WithEnvironment.defaultProps = { | ||
forwardedRef: undefined, | ||
}; | ||
return React.forwardRef((props, ref) => ( | ||
<WithEnvironment | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
forwardedRef={ref} | ||
/> | ||
)); | ||
|
||
return WithEnvironment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better to add this function into a separate file. Also, if this HoC isn't being used at a lot of places, it might be a better idea to just get rid of it. I see that you've already created a hook for this.
return { | ||
environment, | ||
environmentURL, | ||
isProduction: environment === CONST.ENVIRONMENT.PRODUCTION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be part of the context value as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be happy to address this in a follow-up 👍🏼
I realize that this makes it so that the hook has to copy the object from the return value of useContext
every time, which is a waste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work @roryabraham!
I have left some minor comments. However, you can merge this if you feel like those are not urgent enough to be addressed.
@MonilBhavsar 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
TIL about and thanks for the comments @allroundexperts. I think there's a number of good thoughts there but it's more important to get this merged so I can implement the other pages soon here. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.42-0 🚀
|
<View style={[styles.reportOptions, styles.flexRow, styles.pr5, styles.alignItemsCenter]}> | ||
{shouldShowDownloadButton && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactor has caused a deploy blocker #23129 as we have forgot to pass the children which was a change made in the deleted file but probably did not cause morege conflicts or it was missed when resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my bad I missed that change when resolving conflicts
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀
|
|
||
...withDelayToggleButtonStatePropTypes, | ||
/** Ref passed to the component by React.forwardRef (do not pass from parent) */ | ||
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(React.Component)})]).isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from #23093: Is there reason to make this prop required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was that since it's always passed by forwardRef
, then it would make sense to have it be required. However I see now that it will be null/undefined if the parent doesn't pass a ref.
As per the checklist, this PR caused this regression #23102. It's explained here, but TLDR is that some code was removed previously and accidentally added back here which broke navigating to the parent report. |
This minor issue was probably missed in the PR. Issue: Preferences - Shows blue color in Bottom Safearea when going offline Steps to reproduce:
|
HOLD on #20663
Details
This PR was split off of #15462. It implements a new layout in the
Settings
->Preferences
page.Fixed Issues
$ (partial) #12261
Tests
Offline tests
Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
web.mov
Mobile Web - Chrome
androidWeb.mov
Mobile Web - Safari
iOSWeb.mov
Desktop
desktop.mov
iOS
iOS.mov
Android
android.mov