Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Extend Portal component with title, buttons & steps (as per Modal) #4392

Merged
merged 19 commits into from
Feb 3, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Feb 1, 2017

  • Closer align the Portal with the Modal (for which it is supposed to be a replacement)
  • DRY up Portal-user, i.e. no ~/ui/ContainerTitle imports, no additional classes for e.g. buttons
  • Center Portal fully on-screen (margin around)
  • Add overflow: hidden to body when Portal is visible
  • Allow the ability to discard onClose when hideClose prop is set (e.g. FirstRun, non-closable)
  • Add ~/ui/Title for rendering between Portal & Modal
  • Consistency between use, i.e. the output looks the same regardless of where it is used

parity 2017-02-01 22-18-53

Future -

This is very much the start of a journey along a road. Future work will be driven by conversion of some of the modals to Portal, the changes here make at least ~/modals/FirstRun operational. (Part of another WIP PR)

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Feb 1, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 2, 2017
@jacogr jacogr changed the title Extend Portal component with title & button props Extend Portal component with title, buttons & steps (as per Modal) Feb 2, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Feb 3, 2017

This feels wrong :

image

I suggested something like this:

diff --git a/js/src/modals/AddDapps/addDapps.js b/js/src/modals/AddDapps/addDapps.js
index 0a4d19a..b91920f 100644
--- a/js/src/modals/AddDapps/addDapps.js
+++ b/js/src/modals/AddDapps/addDapps.js
@@ -41,15 +41,13 @@ export default class AddDapps extends Component {
         className={ styles.modal }
         onClose={ store.closeModal }
         open
+        title={
+          <FormattedMessage
+            id='dapps.add.label'
+            defaultMessage='visible applications'
+          />
+        }
       >
-        <ContainerTitle
-          title={
-            <FormattedMessage
-              id='dapps.add.label'
-              defaultMessage='visible applications'
-            />
-          }
-        />
         <div className={ styles.container }>
           <div className={ styles.warning } />
           {
diff --git a/js/src/ui/Portal/portal.css b/js/src/ui/Portal/portal.css
index a7d650c..72cd621 100644
--- a/js/src/ui/Portal/portal.css
+++ b/js/src/ui/Portal/portal.css
@@ -101,6 +101,10 @@ $popoverZ: 3600;
     }
   }
 
+  .content {
+    flex: 1;
+  }
+
   .closeIcon {
     font-size: 4em;
     position: absolute;
diff --git a/js/src/ui/Portal/portal.js b/js/src/ui/Portal/portal.js
index fea64fd..f93b037 100644
--- a/js/src/ui/Portal/portal.js
+++ b/js/src/ui/Portal/portal.js
@@ -102,7 +102,9 @@ export default class Portal extends Component {
               steps={ steps }
               title={ title }
             />
-            { children }
+            <div className={ styles.content }>
+              { children }
+            </div>
           </div>
         </div>
       </ReactPortal>

@ngotchac ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 3, 2017
steps
? steps[current]
? steps[activeStep]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the first step as default if activeStep is not passed ?

@ngotchac
Copy link
Contributor

ngotchac commented Feb 3, 2017

(Added an example with steps and button in the Playground) => The buttons doesn't have any bottom-padding. Might want to add some

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 3, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Feb 3, 2017

Only remark : now the addDapps__container has a margin-top and the portal title has a margin-bottom. So the result is twice the wanted margin I think

@ngotchac ngotchac added the A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. label Feb 3, 2017
@ngotchac ngotchac removed the A0-pleasereview 🤓 Pull request needs code review. label Feb 3, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Feb 3, 2017

And the Address Selector is broken

@ngotchac
Copy link
Contributor

ngotchac commented Feb 3, 2017

image

@jacogr
Copy link
Contributor Author

jacogr commented Feb 3, 2017

Oompf. Doesn't seem to like the .container idea as proposed then. (Ok, will get to it eventually to actually run through.)

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 3, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 3, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 3, 2017

Fixed selector, removed extra padding.

@jacogr
Copy link
Contributor Author

jacogr commented Feb 3, 2017

One annoyance (regression) issue remaining where the category headers are now scrollable. However, the PR fixes the titles & (very annoying) body scrollbar positions. Logged issue for outstanding issue.

@jacogr jacogr merged commit c7f5ee4 into master Feb 3, 2017
@jacogr jacogr deleted the jg-extend-portal branch February 3, 2017 21:44
jacogr added a commit that referenced this pull request Feb 3, 2017
gavofyork pushed a commit that referenced this pull request Feb 4, 2017
* s/Delete Contract/Forget Contract/ (#4237)

* Adjust the location of the signer snippet (#4155)

* Additional building-block UI components (#4239)

* Currency WIP

* Expand tests

* Pass className

* Add QrCode

* Export new components in ~/ui

* s/this.props.netSymbol/netSymbol/

* Fix import case

* ui/SectionList component (#4292)

* array chunking utility

* add SectionList component

* Add TODOs to indicate possible future work

* Add missing overlay style (as used in dapps at present)

* Add a Playground for the UI Components (#4301)

* Playground // WIP

* Linting

* Add Examples with code

* CSS Linting

* Linting

* Add Connected Currency Symbol

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* Added `renderSymbol` tests

* PR grumbles

* Add Eth and Btc QRCode examples

* 2015-2017

* Add tests for playground

* Fixing tests

* Split Dapp icon into ui/DappIcon (#4308)

* Add QrCode & Copy to ShapeShift (#4322)

* Extract CopyIcon to ~/ui/Icons

* Add copy & QrCode address

* Default size 4

* Add bitcoin: link

* use protocol links applicable to coin exchanged

* Remove .only

* Display QrCode for accounts, addresses & contracts (#4329)

* Allow Portal to be used as top-level modal (#4338)

* Portal

* Allow Portal to be used in as both top-level and popover

* modal/popover variable naming

* export Portal in ~/ui

* Properly handle optional onKeyDown

* Add simple Playground Example

* Add proper event listener to Portal (#4359)

* Display AccountCard name via IdentityName (#4235)

* Fix signing (#4363)

* Dapp Account Selection & Defaults (#4355)

* Add parity_defaultAccount RPC (with subscription) (#4383)

* Default Account selector in Signer overlay (#4375)

* Typo, fixes #4271 (#4391)

* Fix ParityBar account selection overflows (#4405)

* Available Dapp selection alignment with Permissions (Portal) (#4374)

* registry dapp: make lookup use lower case (#4409)

* Dapps use defaultAccount instead of own selectors (#4386)

* Poll for defaultAccount to update dapp & overlay subscriptions (#4417)

* Poll for defaultAccount (Fixes #4413)

* Fix nextTimeout on catch

* Store timers

* Re-enable default updates on change detection

* Add block & timestamp conditions to Signer (#4411)

* Extension installation overlay (#4423)

* Extension installation overlay

* Pr gumbles

* Spelling

* Update Chrome URL

* Fix for non-included jsonrpc

* Extend Portal component (as per Modal) #4392
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants