-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add FullPageNotFoundView #9649
Add FullPageNotFoundView #9649
Conversation
I'm still waiting on text from marketing (and translation) for this page but the code is ready for review in the meantime cc @Luke9389 @roryabraham since you guys were on the PR for the blocking offline page |
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.
Otherwise LGTM 👍
Updated with default props and finalized text for english and spanish! |
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.
Just some very minor comments, overall LGTM 👍
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.
LGTM 👍
Triggered auto assignment to @Justicea83 ( |
@arosiclair looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
Not sure how this is triggered, but this PR was tested on all platforms minus Android. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
why not android? |
Android build is currently broken on M1. @Justicea83 You don't have an intel mac you could test this with do you? |
Nope, I use M1 |
🚀 Deployed to staging by @arosiclair in version: 1.1.81-0 🚀
|
Alright I was able to get the android build on my other laptop. Tested and added a screenshot. @Justicea83 can you do internal QA to verify and check off this PR on the deploy checklist? |
I should have noticed that the QA steps don't work here. The testing steps require making code changes that we can't make on staging |
Since this PR just creates some new components but does not use them anywhere, I'm tempted to say it's |
Oh I didn't know that InternalQA meant explicitly testing on staging. But yeah with the ad-hoc testing me and Justice have done so far I think this is good to go with no further QA |
🚀 Deployed to production by @roryabraham in version: 1.1.82-5 🚀
|
Refactors
FullPageOfflineBlockingView
to use a sharedBlockingView
component and addsFullPageNotFoundView
for use when the user navigates to a resource that doesn't exist or they have lost access to.The not found view replaces the content when the
shouldShow
prop istrue
.Fixed Issues
$ #9522
Tests
ScrollView
in InitialSettingsPage.js with the newFullPageNotFoundView
shouldShow
prop tofalse
or omit it.shouldShow
prop totrue
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
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)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
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)QA Steps
Same as Tests
Screenshots
Web
Mobile Web
Desktop
iOS
Android