Skip to content
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

Migrate to Baseui #22

Closed
Firenze11 opened this issue Sep 10, 2019 · 5 comments
Closed

Migrate to Baseui #22

Firenze11 opened this issue Sep 10, 2019 · 5 comments
Labels
rfc Request for comments

Comments

@Firenze11
Copy link
Contributor

Firenze11 commented Sep 10, 2019

Currently, within Manifold package we are using styled-components to do custom styling; it becomes complicated for advanced UI widgets (e.g. select with searching functionalities) and hinders development speed.

Pros:

  • Using standard UI widget will shorten the development time of UI part and ensures we focus on visualization part
  • Helps the project to scale in the long run -- developers who are familiar with Baseui can easily jump in
  • Aligns with other Uber UI product

Cons:

  • Need to refactor in the short term
  • Not good for package size -- Baseui is using styletron as engine; Kepler.gl is using styled-components, and if we switch, Manifold will be depending on both (it seems not hard for Baseui to support styled-components, but it's depriotized https://github.com/uber-web/baseui/issues/114)

Middle-ground solution: we could keep styled-components for e.g. styling divs and only use Baseui for UI widgets for now, so that the refactoring work will not involve refactoring styled-component css-like strings to styletron objects:

// styled-components
const Centered = styled.div`
  display: flex;
  justify-content: center;
`;
// styletron
const Centered = styled('div', {
  display: 'flex',
  justifyContent: 'center',
});
@Firenze11 Firenze11 added the rfc Request for comments label Sep 10, 2019
@Firenze11 Firenze11 changed the title [rfc] Migrate to Baseui Migrate to Baseui Sep 10, 2019
@gnavvy
Copy link
Contributor

gnavvy commented Sep 10, 2019

Thanks for the initiative. Regarding package size in Cons:

Per Ryan's comment, styletron-react-core is supposed to be tree-shakeable. Styled-components should also come with tree-shaking support except for a few issues depending on how we write the component (e.g. with static props).

Shall we do around of benchmarking for the bundle size first, and seek help from BaseUI/Kepler.gl if the size is significantly large?

@kenns29
Copy link
Collaborator

kenns29 commented Sep 10, 2019

I'm not a big fan of styled-components. I prefer using UI tools such as baseui or antd and manage css customization through sass or less. You can maintain a namespace scheme to associate each sass file to a component, which basically negates the necessity of using styled components.

Namespace as such:

feature-list-drawing.sass

.feature-list-drawing{
  ...
}

feature-list-drawing.js

import feature-list-drawing.sass

@chrisirhc
Copy link
Contributor

Not good for package size -- Baseui is using styletron as engine; Kepler.gl is using styled-components, and if we switch, Manifold will be depending on both (it seems not hard for Baseui to support styled-components, but it's depriotized uber-web/baseui#114)

How significant is this consideration? If it's not a huge increase in package size in terms of proportion to the current package, then the middle-ground solution seems to be low-hanging to test.

@Firenze11
Copy link
Contributor Author

Current bundle size for the demo website build:

Built at: 2019-09-12 23:19:06
     Asset       Size  Chunks             Chunk Names
 bundle.js   28.1 MiB    main  [emitted]  main
index.html  223 bytes          [emitted]  

@Firenze11
Copy link
Contributor Author

Resolved by #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments
Projects
None yet
Development

No branches or pull requests

4 participants