UX-565 Takeover component migration

[DanilAgafonov]

🛠 Purpose

Takeover component can toggle a full-screen view to help the user focus on complex UI tasks.
More information at the design system site: design.wegalvanize.com/p/components/takeover

Resolves #208

---

✏️ Notes to Reviewer

—

---

🖥 Screenshots

Basic:

With nested SidePanel:

With full-width content:

---

http://storybooks.highbond-s3.com/paprika/UX-565-migrate-takeover

[nahumzs]

Could you add a README.md file explaining the use of the component, the API seems good to me 👍 follows a similar approach with the sidepanel, and how the modal will work.

[nahumzs]

same here in the name of consistency please move this into their own file and use css prop.

[mikrotron]

I'm not 100% convinced, but definitely swayed by Danil's argument against using the css prop.

But I certainly do like separating the styling into a styles.js file.

[nahumzs]

what about then if you want to use style components directly which I'm not against, start implementing import * as styled from "..."

and do <styled.Wrapper> which to me feels better than Wrapper or WrapperStyled

what do you think? (danil, mike)?

cc @mikrotron

[DanilAgafonov]

It feels much better for me, than using <div css={...}>...</div>, but anyway I think it is not how styled-components should be used.

The main idea of SC is remove mapping between styles and components. Every styled component is just normal React component. It is an what author of SC trying to enforce across SC community (he was saying about this many times on twitter and conferences). For example, you can watch Max's (author of SC) talk: https://www.youtube.com/watch?v=jaqDA7Btm3c

Some examples of using styled components from real production grade applications:
https://github.com/outline/outline/blob/master/app/components/DropdownMenu/DropdownMenu.js
https://github.com/withspectrum/spectrum/blob/alpha/src/components/logo/index.js
https://github.com/caspg/space-exp/blob/master/src/components/Header/Header.jsx
https://github.com/prisma-labs/graphql-playground/blob/master/packages/graphql-playground-react/src/components/Settings.tsx

[nahumzs]

When we started the paprika repository, we decided to go with the css approach since we didn't want to enforce all our developer to jump into styled-component was just too disruptive at that time, but now I think its safe to start using styled instead css whenever make sense or the majority of the time.

The only thing that I don't understand from your comment is:

but anyway I think it is not how styled-components should be used.

We are not mapping anything we are just describing what kind of component it's, it's the same that declaring it on the file, but in our case, we want to split it into a different file, so we can distinguish which is a styled component from a regular component.

// Some.styles.js
export const MyStuff = styled.div`` 
// or
export const MyStuff = styled(Some)``

// Some.js
import * as styled from "./Some.styles.js"

return <styled.MyStuff>content</styled.MyStuff>

you still could import MyStuff at any file and will keep the styling and make it a Styled component without css.
From how I see it the only thing we are not doing is declaring it in the same file. or am I missing something?

Ps. https://github.com/prisma-labs/graphql-playground/blob/master/packages/graphql-playground-react/src/components/Settings.tsx#L14

^ that is the kind of declaration I want to avoid, Wrapper, IconWrapper, IMO read better and fits better using <styled.Wrapper /> and <styled.Icon />

///
https://github.com/caspg/space-exp/blob/master/src/components/Header/Header.jsx#L59
^ this one as well is not describing that is a styled component.

CC @mikrotron

[DanilAgafonov]

Thanks, @nahumzs! I got your point. I will refactor this PR to export styled components from *.styles.js files to move forward, but suggest to continue this discussion.

---

From my point of view, sometimes it is hard to say whether component should be placed in *.styles.js file or should be placed like regular React component.

Examples:

1. https://github.com/acl-services/paprika/blob/f9443c6f4126fcf4f110154386269664f5f6efab/packages/Takeover/src/components/Content.js

2. const Checkbox = styled.input.attrs({
     type: 'checkbox',
   })`
     margin: 0 0.7em 0 0;
   `;

3. const SignInLink = styled(ButtonLink).attrs({
     to: '/signin',
     children: 'Sign In',
     neutral: true,
     inline: true,
   })`
     margin-top: 1em;
   `;

4. const RequiredMark = styled.span.attrs({
     children: '*',
   })`
     color: #ff0000;
   `;

For me it is hard to say should I put this components to *.styles.js file or not, because this components uses SC but not only adds styles:

1. Content.js contains SC, but this SC exposed as <Takeover.Content>. This is why it can't be placed in Takeover.styles.js
2. Checkbox not only adds styles. It declares type attr for <input>
3. SignInLink not only adds styles. It adds some props to base component.
4. RequiredMark no only adds styles. It declares children for <span>.

we want to split it into a different file, so we can distinguish which is a styled component from a regular component.

Is there any reason to have an ability to distinguish which is a styled component from a regular component?
It looks like "Separation of technologies", which doesn't look like best practice in React ecosystem. It would be better to follow "Separation of concerns".

Moreover, I think SC is implementation details. When we use component, we shouldn't know whether it implemented with SC or not. *.styles.js suffix in file name exposes implementation details.

For example:

1. Say, we have RequiredMark. This component is a part of SignUpForm.
2. Say, we put all styled components used for SignUpForm to SignUpForm.styles.js.
3. And now we use it as <styled.RequiredMark>.
4. In some day we decided to add tooltip for RequiredMark.
5. And now RequiredMark looks like:

   const Asterisk = styled.span.attrs({
     children: '*',
   })`
     color: #ff0000;
   `;
   
   const RequiredMark = (props) => (
     <Tooltip text="This field is required">
    	<Asterisk {...props} />
     </Tooltip>
   );

Should RequiredMark be extracted from SignUpForm.styles.js? If yes, than we should change <styled.RequiredMark> to <RequiredMark>. This is what I want to avoid: we don't want to change all usages on changing implementation of used component.

[DanilAgafonov]

By the way, how should I name files with SC? *.styles.js or *.styled.js?

[nahumzs]

About your question *.styles.js.

Lots of good comments Danil.

Definitely we are not trying to do a separation of concerns or technologies, I think the intention was/is to help other developers to find faster where are the styles declare and where is the logic more than separating the technologies. That's all.

I understand your concerns and they are valid. Also, those might be specific cases. The 80/90% of the time, we wouldn't have to worry about those, and when those happen I don't see anything wrong about implementing it as you suggested.

I think most of the things we follow on paprika are guidelines, we are not trying to impose the decision. And most of our decision are trying to solve 80%/90% of the common cases.

cc @mikrotron