Modal components refactoring #2515

guilhermemntt · 2020-06-05T09:26:37Z

This PR is part of the effort to update Decrediton to modern React code using hooks and functional components (#2438) and to increase modularity using specific CSS modules also (#2439). Generally, modals have no or few dynamic controls, so some of them have been moved directly to app/components/modals/ without the need for a specific subdirectory. The latest modals implemented as class components have been refactored to functional components. The style rules will be located in Modals.modules.css in the same directory. I am totally open to suggestions, especially regarding the organization of the files inner app/components/modals/.

Before:

After:

guilhermemntt · 2020-06-07T23:24:14Z

Branch's history has been dated to make it easier to gradual review and merge. Working is still in progress on trezor modals.

amass01

@guilhermemntt Code look good, thanks.
Added some inline comments.

amass01 · 2020-06-08T13:52:13Z

app/components/modals/AddAccountModal/AddAccountModal.jsx

+import Modal from "./AddAccountModalContent";
+import useAddAccountModal from "./hooks";
+
+function AddAccountModal({ onCancelModal, onSubmit, ...props }) {


I would recommend using const with arrow function here and for all other modals

amass01 · 2020-06-08T13:56:17Z

app/components/modals/ChangePassphraseModal/ChangePassphraseModal.jsx

+    updatePrivatePassphrase,
+    updateConfirmPrivatePassphrase,
+    onTriggerPassphraseModalSubmit,
+    ...state


Where is this statedefined?

This is repeating in all modals, is it needed? the hooks seem to return all the values, so not sure if this needed.

The state variables are the hook states used in ***Content.jsx components and/or their children. The unused states in their children components aren't being returned from hooks.js. It can be changed if returned values from hooks are declared and passed as props explicitly as well. I'm open to suggestions here.

I prefer declare and explicit call them, because it makes it easier to know what props a component is using. Also, with hooks there are no more states declaration, so I think it may be confunsing.

amass01 · 2020-06-08T13:58:37Z

app/components/modals/ChangePassphraseModal/hooks.js

+    setConfirmPrivPassError(privPass !== confirmPrivPass);
+  }, [privPass, confirmPrivPass]);
+
+  const isValid = useCallback(() => {


use useMemo here as it returns an object and not a function, same for all hooks, some of them include variables which should be wrapped with useMemo instead of useCallback

Fixed, as the old Modals.js no longer depends on it as a function.

guilhermemntt · 2020-06-10T08:53:15Z

Modals not yet refactored may have some layout bugs since Modals.modules.css is gradually being built , the same can happen with those shown on the GetStarted page due to Settings.less CSS rules. Modals.less will be removed after all the process.

guilhermemntt · 2020-06-18T03:24:25Z

I'm opening this PR to merge as tests with Trezor require previous corrections due to #2539. Some points here:

There are some changes in Settings.less.
Modals.module.css has been imported in:
- app/components/views/AccountsPage/Accounts/Page.js
- app/components/views/SettingsPage/MiscSettings.js
- app/components/views/TicketsPage/PurchaseTab/Tickets.js
  These CSS rules can be moved to the equivalent CSS files during their refactoring. Doing that, these imports will no longer be needed.
I'll open an issue regarding Trezor modals refactoring and I intend to close it after the fix in Authentication with Trezor fails when sending transaction #2539, as I already have the code, though it was not tested yet.

amass01

I noticed you import modal.module.css in many views and component which aren't really related to the modals (left inline comments but I guess there are more in other components).

Please create local css modules for all these files and move the styling away from modal.module.css, actually this is one of the main reasons why css modules exists and they are much predictable & encapsulated approach.

Will do another review cycle when done.

Edit: just saw your comment about that, let's just create a module css without fully migrating the components, just copy over the ones you already have in modal.module.css and all the rest migration work will come later.

amass01 · 2020-06-18T13:46:07Z

app/components/views/TicketsPage/PurchaseTab/Tickets.js

@@ -7,11 +7,12 @@ import { ShowWarning, Subtitle } from "shared";
 import "style/PurchaseTickets.less";
 import { InfoDocModalButton } from "buttons";
 import UnsignedTickets from "./UnsignedTickets";
+import modalStyle from "../../../modals/Modals.module.css";


Please create a module for TicketsPage and use it here, this import doesn't make sense here

amass01 · 2020-06-18T13:47:14Z

app/components/views/SettingsPage/MiscSettings.js

@@ -1,6 +1,7 @@
 import { FormattedMessage as T } from "react-intl";
 import { SettingsInput, NumericInput } from "inputs";
 import { InfoDocFieldModalButton } from "buttons";
+import modalStyle from "../../modals/Modals.module.css";


same here, but as i'm already refactoring this part please leave a TODO and I could handle it later

guilhermemntt · 2020-06-22T10:27:10Z

I've implemented the changes. It seems to be ok now.

vctt94

Thanks for this PR! Good job on it. I have some small nits, but looks really good overall.

Looks like there are some modals which are not properly working.

Ticket auto buyer
VSP remove confirmation

Keep in mind in decrediton we have a right alignment design #1940, Keep that on our modals. I noticed some of them are with the button aligned at the left. Also the import redeem script modal is a bit off alignment as well:

Also I think we could have a copy to clipboard button at the confirm redeem script modal, but not really related to this PR:

The copy seed modal looks too wide:

Here is its spec: #2223

vctt94 · 2020-06-25T18:26:58Z

app/components/modals/ChangePassphraseModal/ChangePassphraseModal.jsx

+    updatePrivatePassphrase,
+    updateConfirmPrivatePassphrase,
+    onTriggerPassphraseModalSubmit,
+    ...state


I prefer declare and explicit call them, because it makes it easier to know what props a component is using. Also, with hooks there are no more states declaration, so I think it may be confunsing.

Refactor ChangePassphraseModal Refactor ImportScriptModal

Refactor PassphraseModal Refactor Modal

Refactor BackupRedeemScriptModal Refactor AddMixerAccountsModal Refactor AutobuyerRunningModal

Refactor DocumentationInfoModal Refactor InfoConfirmModal Refactor InfoModal

Remove unnecessary CSS class rules

guilhermemntt · 2020-07-02T16:51:07Z

@vctt94, I've created a remove VSP confirmation modal:

and opened the issue #2547.

To keep the standard, some lines have been added in DecredConstitution.md and ScriptNotRedeemable.md files by adding modal titles to these files. For now, these changes can produce some visual inconsistencies when using other translations.

In general, other GUI changes have been implemented in Info modals, according to #1940.
The example below shows the result of what have been done:

matheusd · 2020-07-06T13:25:57Z

app/components/modals/AboutModal.jsx

+          />{" "}
+          <a
+            onClick={() =>
+              shell.openExternal("https://decred.org/contributors/")


Given this wasn't a pure file move, I think you should go ahead and replace these inline links for the ExternalLink component

matheusd · 2020-07-06T13:26:07Z

app/components/modals/AboutModal.jsx

+          )}
+        </div>
+        <div className={style.aboutBottomAreaMiddle}>
+          Copyright &copy; 2019{" "}


Add useAboutModal custom hook

vctt94

Thanks! LGTM.

guilhermemntt force-pushed the modalsCSSModules branch from eb5cc45 to acedb7e Compare June 7, 2020 23:17

guilhermemntt marked this pull request as ready for review June 7, 2020 23:24

guilhermemntt marked this pull request as draft June 8, 2020 01:06

amass01 reviewed Jun 8, 2020

View reviewed changes

guilhermemntt force-pushed the modalsCSSModules branch from c358ed6 to 8a4aa70 Compare June 10, 2020 08:15

guilhermemntt force-pushed the modalsCSSModules branch from 4b61864 to ac3685f Compare June 16, 2020 11:12

guilhermemntt marked this pull request as ready for review June 18, 2020 03:24

amass01 suggested changes Jun 18, 2020

View reviewed changes

vctt94 reviewed Jun 25, 2020

View reviewed changes

guilhermemntt added 13 commits June 30, 2020 15:45

Refactor AddAccountModal

efda9d1

Refactor ChangePassphraseModal Refactor ImportScriptModal

Replace state object by individual state values

6c4dba9

Fix lint issues

14f93ed

Refactor SeedCopyConfirmModal

0594366

Refactor PassphraseModal Refactor Modal

Refactor AboutModal

d5c5f25

Refactor BackupRedeemScriptModal Refactor AddMixerAccountsModal Refactor AutobuyerRunningModal

Refactor ConfirmModal

be46343

Refactor DocumentationInfoModal Refactor InfoConfirmModal Refactor InfoModal

Remove Modals.module.css imports from non-modal components

ce0e2f1

Remove unnecessary CSS class rules

Declare states explicitly when returning from hook

0d99bc3

Add remove configured VSP confirmation modal

2cf6166

Fix InfoModal style issues

f441c64

Insert modal title in the respective .md file

8ba89d5

Add gap modal specific className

431d422

Refactor some modals to adapt them to the standard

8330814

guilhermemntt force-pushed the modalsCSSModules branch from 7fc646c to 8330814 Compare July 1, 2020 18:39

guilhermemntt added 2 commits July 1, 2020 15:57

Resolve i18n duplicates

ef90811

Rename CSS classes to avoid sintax redundancy using CSS modules

ffb1671

matheusd reviewed Jul 6, 2020

View reviewed changes

Delete helPage connector

253e93b

Add useAboutModal custom hook

vctt94 approved these changes Jul 7, 2020

View reviewed changes

alexlyp merged commit 25191eb into decred:master Jul 7, 2020

guilhermemntt mentioned this pull request Nov 17, 2020

Trezor modals improvements #2956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modal components refactoring #2515

Modal components refactoring #2515

guilhermemntt commented Jun 5, 2020 •

edited

Loading

guilhermemntt commented Jun 7, 2020

amass01 left a comment

amass01 Jun 8, 2020 •

edited

Loading

guilhermemntt Jun 10, 2020

amass01 Jun 8, 2020

amass01 Jun 8, 2020

guilhermemntt Jun 10, 2020

vctt94 Jun 25, 2020

amass01 Jun 8, 2020

guilhermemntt Jun 18, 2020

guilhermemntt commented Jun 10, 2020

guilhermemntt commented Jun 18, 2020

amass01 left a comment •

edited

Loading

amass01 Jun 18, 2020 •

edited

Loading

amass01 Jun 18, 2020

guilhermemntt commented Jun 22, 2020

vctt94 left a comment

vctt94 Jun 25, 2020

guilhermemntt commented Jul 2, 2020

matheusd Jul 6, 2020

guilhermemntt Jul 7, 2020

matheusd Jul 6, 2020

guilhermemntt Jul 7, 2020

vctt94 left a comment

Modal components refactoring #2515

Modal components refactoring #2515

Conversation

guilhermemntt commented Jun 5, 2020 • edited Loading

guilhermemntt commented Jun 7, 2020

amass01 left a comment

Choose a reason for hiding this comment

amass01 Jun 8, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermemntt commented Jun 10, 2020

guilhermemntt commented Jun 18, 2020

amass01 left a comment • edited Loading

Choose a reason for hiding this comment

amass01 Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermemntt commented Jun 22, 2020

vctt94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermemntt commented Jul 2, 2020

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

vctt94 left a comment

Choose a reason for hiding this comment

guilhermemntt commented Jun 5, 2020 •

edited

Loading

amass01 Jun 8, 2020 •

edited

Loading

amass01 left a comment •

edited

Loading

amass01 Jun 18, 2020 •

edited

Loading