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

Remove style-specific components #393

Closed
okdistribute opened this issue Dec 6, 2018 · 5 comments
Closed

Remove style-specific components #393

okdistribute opened this issue Dec 6, 2018 · 5 comments

Comments

@okdistribute
Copy link
Contributor

okdistribute commented Dec 6, 2018

The style of creating helper components such as Unselectable and Centered will cause bloat over time and unnecessary nested divs which make it more difficult to test and navigate through the DOM hierarchy.

For example, in this section, we might want to create instead a single component called Window and include the unselectable and centered style properties in that component as simply css.

https://github.com/deltachat/deltachat-desktop/blob/master/src/renderer/components/SplittedChatListAndView.js#L159-L161

This does create css redundancy across different components but this is a tradeoff of abstraction. When creating an abstraction, we need to ask ourselves if the abstraction is more or just as much code as simply copying the css? If so, the abstraction is not worth it and can cause more confusion for how nested css properties are interacting with each other within a component.

In this case, the window class has no "window" (no pun intended) to see that it is also affected by Unselectable and Centered.

@Jikstra
Copy link
Contributor

Jikstra commented Dec 8, 2018

Two ideas:

  1. use global css classes like .centered or .unclickable and just add them to the component <SomeStyledComponent className="centered unclickable">...</...> Cons, a preprocessor would be maybe worth it if we're using global css helpers.
  2. Use extending of styles https://www.styled-components.com/docs/basics#extending-styles. Cons: It's only possible extend from one element.

I would go for 1.

@okdistribute
Copy link
Contributor Author

I don't think you need these classes at all.

@okdistribute
Copy link
Contributor Author

i.e., just use pure css for centering an item and making something unclickable (it's like, 3 lines of code or less).

@Simon-Laux
Copy link
Member

I vote for the classes instead of components

@okdistribute
Copy link
Contributor Author

Yeah, you can think of components like css ids, they should only be used once or a few times, when a div would be used anyway, and when there is a lot of custom css for that particular component. It really isn't meant to be a replacement for generic css classes, or to wrap basic css like 'unselectable' which is only a couple lines of code and won't be changing

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

No branches or pull requests

3 participants