Add OfflineWithFeedback component #9931

iwiznia · 2022-07-14T21:16:59Z

Details

This PR is just introducing the new component. I tested it in the WorkspaceMembersPage by hardocding the error and pendingAction properties and then removed it before submitting this PR for final review.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/218716

Tests

Checked the component both online and offline and with/without error for :
Pending add
Pending update
Pending delete
Checked component for:
Failed add
Failed update
Failed delete

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

iwiznia · 2022-07-14T22:54:54Z

This is not ready yet, I need to do some more testing, but can you review this in the meantime?

iwiznia · 2022-07-14T22:56:32Z

src/pages/workspace/WorkspaceMembersPage.js

-                        style={[styles.peopleRowCell]}
-                        isChecked={_.contains(this.state.selectedEmployees, item.login)}
-                        disabled={!canBeRemoved}
+            <OfflineWithFeedback error="This is an errors This is an errors This is an errors This is an errors This is an errors This is an errors " style={styles.peopleRowOfflineFeedback} pendingAction="delete">


This is just for testing, I will either remove this from this PR or properly connect it to the Onyx data

src/components/OfflineWithFeedback.js

aldo-expensify · 2022-07-14T23:08:32Z

src/styles/styles.js

+        },
+        error: {
+            flexDirection: 'row',
+            justifyContent: 'center',


NAB: I think this is unnecessary: justifyContent: 'center', but also should not cause bugs

aldo-expensify · 2022-07-14T23:13:52Z

src/styles/styles.js

+            borderColor: themeColors.textError,
+            borderRadius: 6,
+            borderWidth: 2,


At least in web, these border properties seem to be unnecessary (except for borderRadius )

I copied them from somewhere else... will do some tests in native and see if they make a difference there

One thing with the error dot - I think we want to wrap it in a view that has 2px padding. This way the total bound of the red dot is 16x16, which will match other small icons we use for this kind of icon + message pattern. So essentially we'd get something like this:

Let me know what you think about that. I know this isn't an "icon" per se, but all of our icons have a little bit of this white space around it.

Actually, that being said, what we might want to do is just supply this as an .svg and then serve it up just like the rest of the icon system? Thoughts?

I am unsure. I would think is better to do this than using svgs if the effect is the same?

But this would then become almost like an "exception" to the icon system, ya know?

Ah ok, consistency then. I am fine with making it SVG, but have no idea how to do that, any tips?

I got you! dot-indicator.svg.zip

So then just make sure this shows at 16x16, and uses red (danger) for the fill color.

Ah thanks! So I drop this in /assets/images?

I believe so, you should see a bunch of other .svgs there

src/styles/styles.js

src/components/OfflineWithFeedback.js

+    style: [],
+};
+
+function applyStrikeThrough(children) {


Julesssss · 2022-07-19T09:21:26Z

I saw you we're struggling to test Android, so I tried to build it myself, but it looks like this PR is missing the indicator.svg. When it's out of WIP I'll give it another try

iwiznia · 2022-07-19T19:17:41Z

Thanks @Julesssss! Yes, I forgot to push that out, it's pushed now.
And I was able to test on android, the problem now is that it does not work 😄

aldo-expensify

Looks good to me. Would be good to have some instructions on how to test this now that you removed the hardcoded use case from WorkspaceMembersPage

src/pages/workspace/WorkspaceMembersPage.js

Justicea83 · 2022-07-20T09:05:07Z

src/components/OfflineWithFeedback.js

+    error: PropTypes.string,
+
+    /** A function to run when the X button next to the error is clicked */
+    onClose: PropTypes.func.isRequired,


What happens when I don't want anything to happen when I click the close button, do I need to strictly pass a method that does nothing?

I would think that you always want to do something with the X button. It would be weird to have an X button that does nothing :P

heh yes, I think you will always have to pass one, though now I realize that maybe we should have this component remove the error box by default and in that case maybe there are actions that do not require an onClose.... though not sure, I will leave this as required, we can remove the required later when we find a case that does not need it.

iwiznia · 2022-07-21T23:37:53Z

Updated to support multiple errors. Please re-review.

Looks good to me. Would be good to have some instructions on how to test this now that you removed the hardcoded use case from WorkspaceMembersPage

Enclose this with:
<OfflineWithFeedback errors={{one: 'one error that is long one error that is long one error that is long one error that is long one error that is long ', two: 'two error', three: 'three error and this one is medium length'}} pendingAction="add">

Then modify that pendingAction manually to update or delete and play around with adding/removing the error property.

aldo-expensify · 2022-07-22T18:52:02Z

Reviewing... meanwhile, I wonder if the errors should be text errors should be aligned to the left instead of centered:

Center (current)	Left

@shawnborton

iwiznia · 2022-07-22T18:53:41Z

Oh good catch, we have them left aligned in the doc, so will switch it.

shawnborton · 2022-07-22T19:04:19Z

Yup, I agree with that

aldo-expensify · 2022-07-22T19:06:22Z

src/components/OfflineWithFeedback.js

+    const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
+    const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !props.errors;
+    let children = props.children;
+    const sortedErrors = _.map(_.sortBy(_.keys(props.errors)), key => props.errors[key]);


Suggested change

const sortedErrors = _.map(_.sortBy(_.keys(props.errors)), key => props.errors[key]);

const sortedErrors = _.chain(props.errors)

.keys()

.sortBy()

.map(key => props.errors[key])

.value();

This may be more readable

iwiznia · 2022-07-22T19:39:07Z

Updated

aldo-expensify · 2022-07-22T19:45:39Z

src/components/OfflineWithFeedback.js

+            {props.errors && (
+                <View style={styles.offlineFeedback.error}>
+                    <View style={styles.offlineFeedback.errorDot}>
+                        <Icon src={Expensicons.DotIndicator} fill={colors.red} height={16} width={16} />


NAB: replace 16 by variables.iconSizeSmall (import variables from '../styles/variables';)

aldo-expensify · 2022-07-22T19:52:17Z

src/components/OfflineWithFeedback.js

+                        <Icon src={Expensicons.DotIndicator} fill={colors.red} height={16} width={16} />
+                    </View>
+                    <View style={styles.offlineFeedback.textContainer}>
+                        {_.map(sortedErrors, (error, i) => (


NAB: because it is hard for me to tell if this has a real impact or not, but it is not recommended by the react doc.

It is not recommended to use indexes (i) as a key, from react doc:

We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state. Check out Robin Pokorny’s article for an in-depth explanation on the negative impacts of using an index as a key. If you choose not to assign an explicit key to list items then React will default to using indexes as keys.

Not sure if it will have a real impact here... but an easy option to it would have been:

const sortedErrorKeys = _.chain(props.errors) .keys() .sortBy() // .map(key => props.errors[key]) Don't map this to values! .value();

then, in your map here you would use the error's key as the key:

{_.map(sortedErrorKeys, (errorKey) => ( <Text key={errorKey} style={styles.offlineFeedback.text}>{props.errors[errorKey]}</Text> ))}

We don’t recommend using indexes for keys if the order of items may change

The order of items will never change, so I think we are fine here, no?

hmm the index for one of the errors may change if one of the errors is deleted

aldo-expensify · 2022-07-22T20:17:55Z

src/components/OfflineWithFeedback.js

+
+const OfflineWithFeedback = (props) => {
+    const isOfflinePendingAction = props.network.isOffline && props.pendingAction;
+    const isUpdateOrDeleteError = props.errors && (props.pendingAction === 'delete' || props.pendingAction === 'update');


NAB: I think you are considering props.errors equal null when there are no errors. Just wondering if this should also consider {} as no errors

maybe this should be left as it is and we should not expect {}... i don't like having two different things representing the same (no errors)

Ah good question... I think you are right and I should be using _.isEmpty. Changing it.

To clarify, when we have fieldErrors (or errorFields don't recall what we settled in), then it's totally possible that this would end up being an empty object (since onyx can't really clear things)

aldo-expensify

Tested different cases in Android native and Web and didn't find any error. Left just some Nabs.

Some pics

The text you can see like Test props: errors: 1 - delete - offline are just me adding a <Text> describing the props being passed to the case just below it.

In case it is of any use, I did the following to test random cases:

const errorsSeed = [
    null,
    {
        two: 'two error',
        three: 'three error and this one is medium length',
        one: 'one error that is long one error that is long one error that is long one error that is long one error that is long ',
    },
    {
        three: 'three error and this one is medium length',
    },
];

const offlineSeed = [true, false];
const pendingActionSeed = ['add', 'update', 'delete'];

const errors = _.sample(errorsSeed);
const offline = _.sample(offlineSeed);
const pendingAction = _.sample(pendingActionSeed);

const caseDescription = `Test props: errors: ${errors !== null ? Object.keys(errors).length : null} - ${pendingAction} - ${offline ? 'offline' : 'online'}`;

return (
    <View>
        <Text>{caseDescription}</Text>
        <OfflineWithFeedback
            pendingAction={pendingAction}
            isOffline={offline}
            errors={errors}
        >
            <Hoverable onHoverIn={() =>....
                ... rest of the code from WorkspaceMembersPage

iwiznia · 2022-07-22T20:47:36Z

In case it is of any use, I did the following to test random cases:

Damn you are smart! I did it manually LIKE A CAVEMAN!

iwiznia · 2022-07-22T20:49:04Z

Updated it again BTW.

iwiznia · 2022-07-22T21:25:38Z

Going to merge this to unblock other issues, if you have more comments, I will address them in a separate PR.

melvin-bot · 2022-07-22T21:25:53Z

@iwiznia looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

iwiznia · 2022-07-22T21:31:57Z

What? Go home Melvin, you are drunk!

But maybe I merged too fast by mistake and it was still running. Anyways, tests passed.

OSBotify · 2022-07-22T21:33:41Z

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

OSBotify · 2022-07-25T18:03:28Z

🚀 Deployed to staging by @iwiznia in version: 1.1.86-0 🚀

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

OSBotify · 2022-08-01T22:11:15Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.86-5 🚀

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

iwiznia self-assigned this Jul 14, 2022

Add OfflineWithFeedback component

915c5a5

iwiznia force-pushed the ionatan_offlineFeedback branch from dca81f3 to 915c5a5 Compare July 14, 2022 21:17

Fix strikethrough and some styling issues

bd5500d

iwiznia requested a review from aldo-expensify July 14, 2022 22:54

iwiznia commented Jul 14, 2022

View reviewed changes

aldo-expensify reviewed Jul 14, 2022

View reviewed changes

src/components/OfflineWithFeedback.js Outdated Show resolved Hide resolved

aldo-expensify reviewed Jul 14, 2022

View reviewed changes

src/styles/styles.js Outdated Show resolved Hide resolved

iwiznia added 2 commits July 14, 2022 17:38

Fix style error, simplify code

706bd19

Remove unnecesary styles, fix delete casse with error

1374146

aldo-expensify reviewed Jul 15, 2022

View reviewed changes

src/components/OfflineWithFeedback.js Outdated Show resolved Hide resolved

iwiznia added 2 commits July 15, 2022 14:36

Make component stateless, add link to readme

d47546c

Use svg and adjust styling a bit

edcdb20

Julesssss reviewed Jul 19, 2022

View reviewed changes

src/components/OfflineWithFeedback.js

style: [],

};

function applyStrikeThrough(children) {

This comment was marked as resolved.

Sign in to view

iwiznia added 2 commits July 19, 2022 10:36

Merge branch 'main' into ionatan_offlineFeedback

99370a3

Add missing asset

db87b98

iwiznia added 2 commits July 19, 2022 15:29

Make OfflineWithFeedback work on the OptionRow and remove test code

9276eb8

Remove unused import

6b382be

aldo-expensify previously approved these changes Jul 19, 2022

View reviewed changes

src/pages/workspace/WorkspaceMembersPage.js Outdated Show resolved Hide resolved

aldo-expensify self-requested a review July 19, 2022 22:00

iwiznia marked this pull request as ready for review July 19, 2022 23:25

iwiznia requested a review from a team as a code owner July 19, 2022 23:25

melvin-bot bot requested review from tgolen and removed request for a team July 19, 2022 23:25

Justicea83 reviewed Jul 20, 2022

View reviewed changes

iwiznia added 2 commits July 21, 2022 18:06

Sort errors

740fd19

Lint

7daf9cb

aldo-expensify reviewed Jul 22, 2022

View reviewed changes

Align errors to the left

045a6da

aldo-expensify reviewed Jul 22, 2022

View reviewed changes

aldo-expensify previously approved these changes Jul 22, 2022

View reviewed changes

Check for errors in a better way

1bb5c7b

iwiznia dismissed aldo-expensify’s stale review via 1bb5c7b July 22, 2022 20:49

aldo-expensify approved these changes Jul 22, 2022

View reviewed changes

iwiznia merged commit 9364784 into main Jul 22, 2022

iwiznia deleted the ionatan_offlineFeedback branch July 22, 2022 21:25

melvin-bot bot added the Emergency label Jul 22, 2022

iwiznia removed the Emergency label Jul 22, 2022

OSBotify mentioned this pull request Jul 25, 2022

Deploy Checklist: New Expensify 2022-07-25 #10077

Closed

92 tasks

shawnborton mentioned this pull request Aug 25, 2022

Refactor Session.resendValidationLink and the ResendValidationForm to use the new API #10556

Merged

92 tasks

-    const sortedErrors = _.map(_.sortBy(_.keys(props.errors)), key => props.errors[key]);
+    const sortedErrors = _.chain(props.errors)
+        .keys()
+        .sortBy()
+        .map(key => props.errors[key])
+        .value();

Add OfflineWithFeedback component #9931

Add OfflineWithFeedback component #9931

Conversation

iwiznia commented Jul 14, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

iwiznia commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Julesssss commented Jul 19, 2022 • edited Loading

iwiznia commented Jul 19, 2022

aldo-expensify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia commented Jul 21, 2022

aldo-expensify commented Jul 22, 2022

iwiznia commented Jul 22, 2022

shawnborton commented Jul 22, 2022

Choose a reason for hiding this comment

iwiznia commented Jul 22, 2022

Choose a reason for hiding this comment

aldo-expensify Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

aldo-expensify left a comment • edited Loading

Choose a reason for hiding this comment

iwiznia commented Jul 22, 2022

iwiznia commented Jul 22, 2022

iwiznia commented Jul 22, 2022

melvin-bot bot commented Jul 22, 2022

iwiznia commented Jul 22, 2022

OSBotify commented Jul 22, 2022

OSBotify commented Jul 25, 2022

OSBotify commented Aug 1, 2022

iwiznia commented Jul 14, 2022 •

edited

Loading

Julesssss commented Jul 19, 2022 •

edited

Loading

aldo-expensify Jul 22, 2022 •

edited

Loading

iwiznia Jul 22, 2022 •

edited

Loading

aldo-expensify left a comment •

edited

Loading