style: Outlined input migration #3566

usame-algan · 2022-02-23T17:23:16Z

What it solves

How this PR fixes it

All input/form fields now use the outlined style instead of filled.

How to test it

Open the Safe app
Interact with every form
Observe that input fields are outlined instead of filled as specified here

Additionally, forms should be tested on Windows to prevent a potential regression with overflowing content within input fields.

Screenshots

…e if not selected

github-actions · 2022-02-23T17:23:33Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-02-23T17:27:10Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-02-23T17:30:36Z

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

coveralls · 2022-02-23T17:33:09Z

Pull Request Test Coverage Report for Build 1927258919

0 of 7 (0.0%) changed or added relevant lines in 4 files are covered.
184 unchanged lines in 23 files lost coverage.
Overall coverage increased (+0.08%) to 35.408%

Changes Missing Coverage	Changed/Added Lines	%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/MethodsDropdown/index.tsx	1	0.0%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/RenderInputParams/InputComponent/ArrayTypeInput.tsx	1	0.0%
src/routes/safe/components/Balances/SendModal/screens/SendCollectible/style.ts	2	0.0%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/MethodsDropdown/style.ts	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/logic/hooks/useEstimateSafeCreationGas.tsx	1	88.0%
src/logic/wallets/pairing/hooks/usePairing.ts	1	0%
src/logic/wallets/pairing/utils.ts	1	53.33%
src/routes/safe/components/Balances/SendModal/screens/SendCollectible/CollectibleSelectField/index.tsx	1	0%
src/routes/safe/components/Balances/SendModal/screens/SendCollectible/TokenSelectField/index.tsx	1	0%
src/routes/safe/components/Settings/SpendingLimit/FormFields/Amount.tsx	1	23.08%
src/components/AppLayout/Header/components/ProviderDetails/PairingDetails.tsx	2	0%
src/logic/notifications/notificationBuilder.tsx	3	30.43%
src/logic/wallets/onboard.ts	3	31.58%
src/components/AppLayout/Header/components/ProviderDetails/ConnectDetails.tsx	4	0%

Totals
Change from base Build 1888234803:	0.08%
Covered Lines:	3405
Relevant Lines:	8680

💛 - Coveralls

github-actions · 2022-02-23T17:40:48Z

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1927296524

Failed tests:

❌ Add an existing safe Add an existing safe
❌ Read-only transaction creation and review Read-only transaction creation and review

iamacook

Nice work on this! Great to see you tweak a few inconsistencies along the way. Did you consult with @yagopv? I don't know if there's any likelihood we could move some of this in his SRC PR perhaps?

You might be able to condense some of the 'extra' styling into global overrides but I'll trust your judgement on that. I'd recommend converting the new MUI styling over to styled-components though.

Side note: there's a missing return type in src/components/forms/SelectField/index.tsx.

src/routes/safe/components/Balances/SendModal/screens/AddressBookInput/style.ts

...utes/safe/components/Balances/SendModal/screens/ContractInteraction/MethodsDropdown/style.ts

iamacook · 2022-02-23T17:51:56Z

src/routes/safe/components/Balances/SendModal/screens/SendCollectible/style.ts

  },
 })
+
+export const selectedTokenStyles = createStyles({


Please see my comment further up about styling in MUI.

This also already existed. I moved it into a new file since it was duplicated before for SendFunds and SendCollectible. We should do the refactor to styled-components in a separate task.

src/routes/safe/components/Balances/SendModal/screens/SendFunds/TokenSelectField/style.ts

iamacook · 2022-02-23T17:53:25Z

src/theme/mui.ts

      },
    },
-    MuiFormHelperText: {
+    MuiFormControl: {


Have you checked for <FormControl> components across the app? When you add new overrides, they will ofc affect all instances already present.

This change replaces the overflowStyle that I deleted where the width was set inline to 100% and overflow:hidden which interferes with the new input labels. There was also a cryptic comment // Neded for solving a fix in Windows browsers so we should make sure to do a full regression test on all form elements. We can still use width: 100% instead of flex: 1 as it is a bit more explicit.

Make sure to add a comment about that to the testing section.

katspaugh

Approved but there are lint issues.

katspaugh · 2022-03-01T14:42:13Z

This lint, I meant:

iamacook

Thanks for addressing my comments. Just some lint left.

iamacook · 2022-03-02T08:04:49Z

The Safe name field in the Safe creation doesn't have a correctly coloured placeholder:

It looks as though the Safe is to be called "Safe name".

The dropdowns are a little skewed too. The numbers don't align quite right:

usame-algan · 2022-03-02T10:02:51Z

The Safe name field in the Safe creation doesn't have a correctly coloured placeholder

This is the label that animates to the top once the input is selected (Figma reference). One alternative we have is forcing the shrink state by default e.g.

We could also remove it altogether in this case since there is already a label above the input field "Name of the new safe"

iamacook · 2022-03-02T10:06:22Z

Are you overriding the label colour? I'd suggest removing that. See the top right example here.

usame-algan · 2022-03-02T10:28:04Z

Are you overriding the label colour? I'd suggest removing that. See the top right example here.

We are overriding the label color as defined in Figma but I don't mind making it a bit lighter. cc @liliiaorlenko

iamacook · 2022-03-02T10:29:59Z

We are overriding the label color as defined in Figma but I don't mind making it a bit lighter.

In this context, I would argue that it is more of a placeholder. I initially thought we had a bug naming the Safe.

github-actions · 2022-03-02T10:35:52Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

francovenica · 2022-03-02T19:08:38Z

Load safe form - Owners list
The names that are autocompleted (whatever those be because ENS primary names or address that already have names in the address book) are being stomped by the input name, in this case the text "owner name"

In this gif you can see that some owners have an ENS name already, but those are considered placeholders. See how when I enter a name manually "test1" the "owner name" label is moved to the top of the input

francovenica · 2022-03-02T19:22:47Z

The new error message outside of the input make the modals wiggle much more when the error is shown or not.
In the original version, since the error message was inside the input the modal would barely stretch the modal when the error was shown

EDIT:
Regarding this, maybe we can make all the inputs like this one "Recipient", where the name is in the upper corner all the time and the error message is shown there instead of showing outside. This is outside of the scope of this ticket, but could be discussed with the team if it is something that can be implemented

usame-algan · 2022-03-03T09:48:50Z

The names that are autocompleted are being stomped by the input name, in this case the text "owner name"

Good catch, I adjusted the owner name input fields to be "shrunk" by default so the ENS Name is always visible

Regarding this, maybe we can make all the inputs like this one "Recipient", where the name is in the upper corner all the time and the error message is shown there instead of showing outside. This is outside of the scope of this ticket, but could be discussed with the team if it is something that can be implemented

I like the idea of keeping the error message at the top so the layout doesn't "jump". Some of our error messages are very unspecific like "Required" and would overwrite the input label which could affect UX negatively so we should discuss this cc @liliiaorlenko

francovenica · 2022-03-03T16:46:47Z

The issue reported with the names during the load safe process was fixed.

For the other comment I think is best to create a new ticket to discuss it so we can pass this one

francovenica · 2022-03-03T19:32:03Z

Created this new ticket to discuss the error message placement #3621. I'll pass this one to QA done

liliiaorlenko · 2022-03-04T08:43:24Z

I dont mind error messages on top, just make it consistent everywhere

usame-algan added 7 commits February 22, 2022 13:23

style: Override MUI outlined input styles

844d034

style: Adjust TxModalWrapper edit screen input styles

9b58f51

style: Adjust SendFunds screen input styles

75f29db

style: Adjust SendCollectible input styles, dispÃlay placeholder valu…

8e7bd81

…e if not selected

style: Adjust Contract interaction inputs

418cfd4

style: Replace text prop with label prop in all Field elements

a8e027c

Merge remote-tracking branch 'origin/dev' into outlined-input-migration

2e5a14d

iamacook reviewed Feb 23, 2022

View reviewed changes

katspaugh approved these changes Feb 25, 2022

View reviewed changes

fix: Linting and extract mui styles to global overrides

635035d

iamacook approved these changes Mar 1, 2022

View reviewed changes

fix: Input elements in safe creation and adjust label colors

2ad449b

usame-algan temporarily deployed to Manual March 2, 2022 10:32 Inactive

fix: Remove redundant label for custom tx value input

a2ec3cd

usame-algan temporarily deployed to Manual March 2, 2022 10:34 Inactive

fix: Shrink owner name labels by default on load safe

e331aaa

usame-algan temporarily deployed to Manual March 3, 2022 09:44 Inactive

francovenica mentioned this pull request Mar 3, 2022

Error messages placement for new inputs #3621

Closed

usame-algan merged commit 10f502c into dev Mar 4, 2022

usame-algan deleted the outlined-input-migration branch March 4, 2022 08:35

github-actions bot locked and limited conversation to collaborators Mar 4, 2022

style: Outlined input migration #3566

style: Outlined input migration #3566

Uh oh!

Conversation

usame-algan commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it solves

How this PR fixes it

How to test it

Screenshots

Uh oh!

github-actions bot commented Feb 23, 2022

Uh oh!

github-actions bot commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

github-actions bot commented Feb 23, 2022

Deployment links

Uh oh!

coveralls commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1927258919

💛 - Coveralls

Uh oh!

github-actions bot commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iamacook Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

usame-algan Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iamacook Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

usame-algan Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

iamacook Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

katspaugh left a comment

Choose a reason for hiding this comment

Uh oh!

katspaugh commented Mar 1, 2022

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

iamacook commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

usame-algan commented Mar 2, 2022

Uh oh!

iamacook commented Mar 2, 2022

Uh oh!

usame-algan commented Mar 2, 2022

Uh oh!

iamacook commented Mar 2, 2022

Uh oh!

github-actions bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

francovenica commented Mar 2, 2022

Uh oh!

francovenica commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

usame-algan commented Mar 3, 2022

Uh oh!

francovenica commented Mar 3, 2022

usame-algan commented Feb 23, 2022 •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

coveralls commented Feb 23, 2022 •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

iamacook commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

francovenica commented Mar 2, 2022 •

edited

Loading