New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Popover and TransactionProgress component #225

Merged
merged 27 commits into from Nov 8, 2018

Conversation

Projects
None yet
3 participants
@deamme
Copy link
Contributor

deamme commented Oct 18, 2018

Closes #220 and #199

This should fix Gitcoin warnings.

@bpierre
Copy link
Member

bpierre left a comment

Hey @deamme, awesome work! 💯 (and sorry for the delay to review this PR 🙇‍♂️)

I only reviewed Popover for now (not TransactionProgress). It works well, I can’t wait to have such a useful component in our toolbox!

I left some comments, and I was also thinking that it would be nice to make it a bit higher level, by having some default styles, and an option to let it closes itself by pressing escape or clicking outside. We could implement the actual features later, but maybe we should start to display it using a callback rather than by rendering it or not? That way, Popover could manage its own visibility.

The API could look like this:

const MyApp = () =>(
  <Popover>
    {(popover) => (
      <button
        onClick={event => {

          // display the popover
          popover(
            event.target,
            <div>Hello from Popover!</div>,
            { /* optional params like `placement` */ }
          )
        }}
      >
        Popover
      </button>
    )}
  </Popover>
)

Tell me what you think!

Show resolved Hide resolved src/components/Popover/Popover.js Outdated
Show resolved Hide resolved src/icons/components/Close.js Outdated
<path
d="M6.155 8.547h1.298V3.3H6.155v5.247zM6.045 11h1.529V9.537H6.045V11z"
fill="#6D777B"
fill={props.color || "#6D777B"}

This comment has been minimized.

@bpierre

bpierre Oct 22, 2018

Member

That’s nice, I think it would be useful to have this on other icons!

The only problem is that these components are generated from the SVG files in icons/svg/, and whatever is changed in icons/components/ will get overridden by npm run icons:build.

But we could use another color in the SVG, so we can replace it by {props.bg || defaultBackground}? We could also replace currentColor by {props.color || 'currentColor'} in the npm script.

Also background would be better than bg (we try to avoid abbreviations when possible).

This comment has been minimized.

@deamme

deamme Oct 23, 2018

Contributor

I don't understand how to fix this..

This comment has been minimized.

@AquiGorka

AquiGorka Nov 6, 2018

Contributor

npm run icons:build uses svgr to generate the react components every time so, I'm thinking we could use a styled-wrapper that defines the default color and allows overrides:

// remove fills from svg
// keep src/components/Attention.js intact
// add StyledAttention.js and use this as the icon default export

const StyledAttention = styled(Attention)`
  svg path{
    fill: ${({ color = '#DAEAEF' }) => color}
  }
`

As the comments suggest, we'll need to remove the fills from the svgs and add new styled wrappers which will become the exports for the react-svg-components.

Thoughts?

This comment has been minimized.

@bpierre

bpierre Nov 6, 2018

Member

I’m going to work on this part, I’m not sure styled-components is needed here but it’s an interesting idea 🤔

I’ll give it a go!

Show resolved Hide resolved src/components/index.js Outdated
Show resolved Hide resolved src/components/Popover/Popover.js Outdated
Show resolved Hide resolved src/components/Popover/Popover.js Outdated
Show resolved Hide resolved src/components/Popover/Popover.js Outdated
Show resolved Hide resolved src/components/Popover/Popover.js Outdated
Show resolved Hide resolved src/components/Popover/README.md
},
],
'@babel/preset-react',
],

This comment has been minimized.

@bpierre

This comment has been minimized.

@deamme

deamme Oct 22, 2018

Contributor

Yeah but didn't seem to work for me unfortunately and that fixed it

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Oct 23, 2018

@bpierre I've tried to do your suggested API but can't really seem to do it since it's a bit complicated when using Portals (they don't work since things are executed inside the onClick event closure I think..). I'm now looking into how to close the Popover when clicking outside of it (apparently more complicated than I initially thought because of the current API). Lastly, I don't understand how to fix the Icon issue..

UPDATE:
I managed to make closing the Popover when clicking outside of it work with the current setup.

UPDATE 2:
It didn't work when wrapping the TransactionProgress component. I'm trying another solution now..

UPDATE 3:
The new solution seems to work!

@deamme deamme force-pushed the deamme:popover-components branch from eb6ca04 to 768acd2 Nov 4, 2018

@bpierre bpierre requested review from AquiGorka and bpierre Nov 5, 2018

{({ scale, opacity }) => (
<Card
style={{
opacity,

This comment has been minimized.

@AquiGorka

AquiGorka Nov 6, 2018

Contributor

When testing the component page locally, the component would not show after clicking the button (kept opacity: 0).
Fixed by updating to: opacity: `${opacity}`

Did this happen to you @deamme? Maybe @bpierre can shed some light here?

This comment has been minimized.

@deamme

deamme Nov 6, 2018

Contributor

I think this has happened to @sohkai too but never happened to me

This comment has been minimized.

@bpierre

bpierre Nov 8, 2018

Member

It happened to me too, not sure why, but it’s fixed since Popover is managing it.

const Base = styled.div`
width: 100%;
height: 8px;
background: #edf3f6;

This comment has been minimized.

@AquiGorka

AquiGorka Nov 6, 2018

Contributor

This color is used in the Slider component too, should we add it to the theme?

This comment has been minimized.

@bpierre

bpierre Nov 8, 2018

Member

There is some work to do on the theme with the move to Lorikeet, so let’s fix the two together, when working on the new palette.

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Nov 8, 2018

@deamme I started adding some changes, I hope you don’t mind!

  • Renamed handleClose to onClose (this is the convention we use: onFoo when it’s a prop, handleX when it’s a method).
  • RootProvider is now Root (+ Root.Provider), and is exported like the other providers.
  • Popover now manages the card style + transition, and TransactionProgress is using it directly, so that users can use TransactionProgress without having to worry about Popover. I think we might want to separate it in the future (e.g. Popover for positioning, and CardPopover for the style), or have a prop, but we can do this later.
  • In the gallery, import the components using @aragon/ui (compiled) rather than ui-src (source), so that we can remove the specific babel configuration.

bpierre added some commits Nov 8, 2018

const { children } = this.props
return (
<Provider value={element}>
<div ref={this.handleRef}>{children}</div>

This comment has been minimized.

@AquiGorka

AquiGorka Nov 8, 2018

Contributor

This provider adds a wrapper div, maybe this will bring issues with styling (we'll need to pay attention to heights).

This comment has been minimized.

@bpierre

bpierre Nov 8, 2018

Member

Good point, it must be the one rare case where we need to create an element, so that we can refer to it. Tested on aragon/aragon and aragon-website, both are working with it since they don’t set any height on body.

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Nov 8, 2018

@bpierre Sounds good! I don't mind at all 😄

bpierre added some commits Nov 8, 2018

@bpierre bpierre merged commit 9edf588 into aragon:master Nov 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Nov 8, 2018

@deamme I just merged it! Thank you for this big piece of work 🎉💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment