Skip to content
This repository was archived by the owner on May 24, 2022. It is now read-only.

Conversation

ltfschoen
Copy link
Contributor

  • Add styled-components dependency to start introducing it to components instead of Sass

  • Wrap App.js with styled-components ThemeProvider and inject fetherTheme prop colours so they are available to child components.

  • Added styled-components' GlobalStyle https://www.styled-components.com/docs/faqs boilerplate as child of ThemeProvider so global styles may be defined here

  • Replace assets/sass/layouts/_wrapper.scss with styled-components including:

    • DivContent is declared in App.js since it is only used here
    • DivWindow is declared in App.js since it is only used here
    • DivWindowContent is declared in shared folder since used by different components
  • Removed assets/sass/layouts/_wrapper.scss. Noting that .connector class is not used

  • Use styled-components in TxForm and TxDetails components (that were introduced in PR feat - Fixes #293. Shows tx fee estimate and total tx amount. See issues in commit details #307).

  • Add a style.js file in the same directory of the component where styled-components are used

  • Add styled-components theme animations folder with a SlideInLeft animation and introduce a theme to TxDetails

  • Add keyframes animation to TxDetails component (i.e. slide-in animation that's used in the details section that appears when you click Details/Hide button on the Send Ether/THIBCoin page)

  • Fix faint style implementation to use rgba with styled-components

  • Note that I decided not to use defaultProps feature of styled-components as it bloats the code too much

  • Note that currently we are using Sass _variables.scss for colour variables, and since many components still use some of the Sass variables we cannot remove them yet.

  • Note that the original branch where this work was done is https://github.com/paritytech/fether/compare/luke-293-show-tx-fee-styled-components?expand=1, but since multiple PRs have since been merged it was easier to just code again using it as reference

  • Remove colours from fether-ui/src/theme that were not being used and integrate into styled-components fetherTheme

  • Add babel-plugin-styled-components library so debugging shows name not hash

Without this babel plugin when you try to debug in Dev Tools > Elements,
it displays the components with the styled-component name shown as a hash i.e.
<div class="sc-EHOje dGUdfn">, whereas previously with CSS/Sass it showed the class name.

This babel plugin displays the style-component name that we specified instead of a
meaningless hash i.e. <div class="App__DivContent-sc-1l7hwf9-0 jhbGUD">, where DivContent
is the name of the styled-component that we created.

…etails.

* Add styled-components dependency to start introducing it to components instead of Sass

* Wrap App.js with styled-components ThemeProvider and inject `fetherTheme` prop colours so they are available to child components.

* Added styled-components' GlobalStyle https://www.styled-components.com/docs/faqs boilerplate as child of ThemeProvider so global styles may be defined here

* Replace assets/sass/layouts/_wrapper.scss with styled-components including:
  * DivContent is declared in App.js since it is only used here
  * DivWindow is declared in App.js since it is only used here
  * DivWindowContent is declared in shared folder since used by different components

* Removed assets/sass/layouts/_wrapper.scss. Noting that `.connector` class is not used

* Use styled-components in TxForm and TxDetails components (that were introduced in PR #307).

* Add a style.js file in the same directory of the component where styled-components are used

* Add styled-components theme animations folder with a SlideInLeft animation and introduce a theme to TxDetails

* Add keyframes animation to TxDetails component (i.e. slide-in animation that's used in the details section that appears when you click Details/Hide button on the Send Ether/THIBCoin page)

* Fix `faint` style implementation to use `rgba` with styled-components

* Note that I decided not to use defaultProps feature of styled-components as it bloats the code too much

* Note that currently we are using Sass _variables.scss for colour variables, and since many components still use some of the Sass variables we cannot remove them yet.

* Note that the original branch where this work was done is https://github.com/paritytech/fether/compare/luke-293-show-tx-fee-styled-components?expand=1, but since multiple PRs have since been merged it was easier to just code again using it as reference

* Disadvantage of styled-components appears to be when you go to debug components in Dev Tools > Elements. The classNames appear as a hash `<div class="sc-EHOje dGUdfn">` instead of the classname!!
…me not hash

Without this babel plugin when you try to debug in Dev Tools > Elements,
it displays the components with the styled-component name shown as a hash i.e.
`<div class="sc-EHOje dGUdfn">`, whereas previously with CSS/Sass it showed the class name.

This babel plugin displays the style-component name that we specified instead of a
meaningless hash i.e. `<div class="App__DivContent-sc-1l7hwf9-0 jhbGUD">`, where `DivContent`
is the name of the styled-component that we created.
Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Looks good! I think there's some folder refactoring to do in order to merge this, see comments

@@ -3,4 +3,8 @@
//
// SPDX-License-Identifier: BSD-3-Clause

export * from './colors';
import styled from 'styled-components';
Copy link
Collaborator

@amaury1093 amaury1093 Jan 22, 2019

Choose a reason for hiding this comment

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

I don't think putting this JS code inside assets/ is a good idea, assets/ are really for static assets, but this JS actually exports React component.

I think we can create a new folder inside src/, called shared/ or theme/ or components/, and put all shared components inside that folder. Probably stuff like Health and RequireHealthOverlay could also go inside that folder.

The only problem is that this shared components folder will play a similar role as fether-ui. So instead of putting all above components inside the new shared components folder, I'm also okay with putting everything inside fether-ui.

@ltfschoen If you have any proposals on how to organize UI components, I'd like to hear.

Copy link
Contributor Author

@ltfschoen ltfschoen Jan 22, 2019

Choose a reason for hiding this comment

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

I've just sent you a link to a file named bem_styled_components.md https://drive.google.com/file/d/13YdbHTPGnTqWPxS5q8vcPn1Zo-UWgWWU/view?usp=sharing

Copy link
Collaborator

@amaury1093 amaury1093 Jan 22, 2019

Choose a reason for hiding this comment

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

I like it, but I think i haven't 100% understood it:

  • src/components/ This will hold stateful components, so all our Accounts, App, Tokens, Send and Whitelist components (and sub-components) will go under here, correct?
  • src/blocks Do you have examples from our project which would go here? I'm not sure I understood this well, but for me, everything in fether-ui would go inside this folder, correct? In this case do we still keep fether-ui?
  • src/elements Yes, good idea, stuff like Button, Clickable, go there.
  • src/modifiers sounds good to me too!

^ is that more/less what you had in mind too?

Also, where would stuff like Health, RequireHealthOverlay (reusable "blocks" that are connected to state) go?

import { createGlobalStyle } from 'styled-components';

// https://www.styled-components.com/docs/faqs
const GlobalStyle = createGlobalStyle`
Copy link
Collaborator

@amaury1093 amaury1093 Jan 22, 2019

Choose a reason for hiding this comment

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

Same thing here, GlobalStyle is a React component, so doesn't go inside assets/


import { keyframes } from 'styled-components';

export const SlideLeftIn = keyframes`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one's trickier, it's not a React component. I don't have any idea, so let's leave it here for now.

@Tbaut
Copy link
Collaborator

Tbaut commented May 7, 2019

Closing as it won't happen any time soon.

@Tbaut Tbaut closed this May 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants