Refactor: Form elements #1315

arielj · 2022-05-15T19:49:03Z

This PR is a refactor around how we use selects, text inputs, and the toggle switch tags.

There were some issues:

we had to copy some extra tags with specific classes in specific order for things to work properly (like to add a text field we had to add a span.setting, then a label with a specific code for RTL, then another element and so on), now each tag has it's own component so to add a select field (with a label and some extra stuff) we can use SelectField, similar with TextInputField, also each component handles the RTL variable
the css for those elements was a big hacky and elements were not all the same width (which was driving me crazy xD)
responsiveness was not asking properly in some cases
for the ToggleSwitch, I removed some elements to make it simpler relying a big more in CSS instead of code
The FormControl element is doing to much and still requires specific html, these new components are more focused

I'll add more comments in the diff

Use the following Checklist if you have changed something on the Backend or Frontend:

Tested the feature and it's working on a current and clean install.
Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
Created / Updated Tests (If necessary)
Created / Updated documentation (If necessary)

arielj · 2022-05-15T19:49:54Z

src/components/UI/LanguageSelector/index.tsx

+  showWeblateLink?: boolean
+}
+
+const languageLabels: { [key: string]: string } = {


moved these 2 constants outside the component so they are not initialized in every call to the function

arielj · 2022-05-15T19:51:31Z

src/components/UI/ToggleSwitch/index.tsx

-    dataTestId = 'toggleSwitch'
-  } = props
-  // TODO fixes errors in the console, but the props may not be necessary at all
-  const checkmarkProps = {


I was able to remove the need of the .checkmark element completely so we don't need this anymore

arielj · 2022-05-15T19:52:10Z

src/components/UI/ToggleSwitch/index.css

-.switch input:checked ~ .checkmark:after {
-  display: block;
+.toggleSwitchWrapper.checked:before {
+  content: '\2713';


replaced the hacky check done using css borders and rotation with a real check character

arielj · 2022-05-15T19:54:45Z

src/screens/Accessibility/index.css

@@ -3,29 +3,36 @@ input[type='range'] {
  max-width: 513px;
 }

-.Accessibility > .settingsWrapper > .setting > .is-drop-down {
-  background-position: calc(100% - 20px) calc(1em + -4px),
-    calc(100% - 15px) calc(1em + -4px), calc(101% - 2.5em) calc(0.5em + -8px);


changed the main background properties so it works in smaller and bigger selects so this is not needed

arielj · 2022-05-15T19:56:59Z

src/screens/Login/index.tsx

-  const handleChangeLanguage = (language: string) => {
-    storage.setItem('language', language)
-    i18n.changeLanguage(language)
-  }


language changes are handled by the LanguageSelector component directly

flavioislima · 2022-05-16T12:22:56Z

src/App.css

@@ -185,17 +185,10 @@ html,
 }

 .material-icons.settings.folder {


One thing, not really related with the PR.
Have you noticed that we are using two different folder icons? one on the settings and another on the InstallModal.
I think we should pick one, maybe the one that we are using on the InstallModal looks a bit better imo.
But we can do that in another task if you think this could increase the scope of this PR.

yeah, I didn't know the reason so I just skipped that, I agree that a single icon in all palces would look better

I'm also thinking about adding another component PathInputField that would take care of the input with the folder icon, we use that in a lot of places, I think it's a good opportunity to simplify that

I extracted a TextInputWithIconField component instead, maybe we can extract a PathInputField in the future, but not all path inputs behave the same so decided to go with a more generic input with icon

flavioislima

Looks pretty good!
I can see all settings finally are well aligned :)

just a question, the ToggleSwitch checkmark background should not be using transparent, or the same color as the setting's background? 🤔

flavioislima · 2022-05-17T07:55:07Z

Another bug I just found was tha the settings are not working for RTL anymore, a param for that would be necessary to change the layout for those languages.

will ask changes

arielj · 2022-05-17T11:16:38Z

Looks pretty good! I can see all settings finally are well aligned :)

just a question, the ToggleSwitch checkmark background should not be using transparent, or the same color as the setting's background? thinking

I have to check this, I had an issue with hidding the original input and I was covering it setting the background of the check box, but I fixed that problem and forgot to remove that extra background

arielj · 2022-05-17T11:17:34Z

Another bug I just found was tha the settings are not working for RTL anymore, a param for that would be necessary to change the layout for those languages.

weird, the compontents itself now gets the isRTL from the context so the prop is not needed, maybe I'm not setting that properly, I'll check that in a moment

arielj · 2022-05-17T22:48:31Z

Fixed the RTL language style (I also improved some elements to do full RTL alignment and not just the label

arielj · 2022-05-17T22:50:13Z

Also added an extra TextFieldWithIconField component so we don't need to pass specific markup for the icon, we just pass the icon and the click handler and it will create the correct html.

Finally, I replaced all uses of a Folder icon with the one in the InstallDialog component

flavioislima

I think that looks great now!
Ready to merge! ⚔️

arielj added 4 commits May 15, 2022 14:14

Refactor text/select/switch elements

6d0a131

Fix overflow text behind icon

fc1a366

Refactor ToggleSwitch

0b1676b

SelectField and TextInputField + refactor

e024765

arielj commented May 15, 2022

View reviewed changes

arielj added 2 commits May 15, 2022 17:02

Merge branch 'main' into refactor/form-elements

06b267d

Fix size of recently played to show

5106087

arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label May 15, 2022

arielj added 3 commits May 15, 2022 18:47

Replace another input with the new TextInputField component

621a6dd

Make form elements more generic, don't hardcode setting class

cb7b475

Remove unused css

67d7655

flavioislima reviewed May 16, 2022

View reviewed changes

flavioislima previously approved these changes May 17, 2022

View reviewed changes

Merge branch 'main' into refactor/form-elements

50bb3ce

arielj added pr:wip WIP, don't merge. and removed pr:ready-for-review Feature-complete, ready for the grind! :P labels May 17, 2022

arielj added 2 commits May 17, 2022 19:39

Extract TextInputWithIconField component, fix RTL

766fcbf

Use same folder icon everywhere

aa6f82f

arielj added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:wip WIP, don't merge. labels May 17, 2022

arielj added 5 commits May 17, 2022 19:55

Fix checked toggle state

eb282ac

Handle toggled state with css selector instead of class

5dacf3a

Remove debugger code

0e84b65

Fix width of range field in accessiblity screen

f8710ea

Fix margins of Fields after refactor

ba34130

flavioislima approved these changes May 18, 2022

View reviewed changes

flavioislima added pr:ready-to-merge This PR is fully ready for merge. and removed pr:ready-for-review Feature-complete, ready for the grind! :P labels May 18, 2022

arielj added 2 commits May 18, 2022 20:09

Merge branch 'main' into refactor/form-elements

0c04cb8

Fix missing ID after merge

65c74dc

arielj merged commit d005767 into main May 19, 2022

arielj deleted the refactor/form-elements branch May 19, 2022 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Form elements #1315

Refactor: Form elements #1315

arielj commented May 15, 2022

arielj May 15, 2022

arielj May 15, 2022

arielj May 15, 2022

arielj May 15, 2022

arielj May 15, 2022

flavioislima May 16, 2022

arielj May 17, 2022

arielj May 17, 2022

flavioislima left a comment

flavioislima commented May 17, 2022

arielj commented May 17, 2022

arielj commented May 17, 2022

arielj commented May 17, 2022

arielj commented May 17, 2022

flavioislima left a comment

		@@ -185,17 +185,10 @@ html,
		}

		.material-icons.settings.folder {

Refactor: Form elements #1315

Refactor: Form elements #1315

Conversation

arielj commented May 15, 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

flavioislima left a comment

Choose a reason for hiding this comment

flavioislima commented May 17, 2022

arielj commented May 17, 2022

arielj commented May 17, 2022

arielj commented May 17, 2022

arielj commented May 17, 2022

flavioislima left a comment

Choose a reason for hiding this comment