-
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 offline UX for workspace #10749
Add offline UX for workspace #10749
Conversation
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.
Looking solid! Nice work, just one comment
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.
Tested 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.
Nice!
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.
Changes LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@@ -52,6 +53,9 @@ const propTypes = { | |||
|
|||
/** The user's role in the policy */ | |||
role: PropTypes.string, | |||
|
|||
/** The current action that is waiting to happen on the policy */ | |||
pendingAction: PropTypes.oneOf(['add', 'update', 'delete']), |
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.
There are constants for these
Lines 788 to 792 in 3fa24e2
ADD: 'add', | |
DELETE: 'delete', | |
UPDATE: 'update', | |
}, | |
BRICK_ROAD_INDICATOR_STATUS: { |
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.
Good call. Will update.
@@ -168,13 +173,55 @@ class InitialSettingsPage extends React.Component { | |||
iconFill: themeColors.iconReversed, | |||
fallbackIcon: Expensicons.FallbackWorkspaceAvatar, | |||
brickRoadIndicator: PolicyUtils.getPolicyBrickRoadIndicatorStatus(policy, this.props.policyMembers), | |||
pendingAction: policy.pendingAction ? policy.pendingAction : null, |
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.
The ternary here is useless, no?
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 good point. I was thinking it would throw an error if undefined, but looks like that is not the case.
🚀 Deployed to staging by @Justicea83 in version: 1.1.97-0 🚀
|
Details
Adds offline UX for workspace creation by adding the
OfflineWithFeedback
wrapper on the workspaceMenuItem
in theInitialSettingsPage
and on the entire page inWorkspaceInitialPage
.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/224686
Tests
Onyx.merge('policy_8C6BF17C3E92D600', {pendingAction: 'add'});
in order to create a pending action on your workspacePR 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
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** 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
This won't be testable on staging/production until https://github.com/Expensify/Expensify/issues/224690 is also live. At that point:
+
button and create a workspaceScreenshots
Web
Mobile Web
Desktop
iOS
Android