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

Refactor CSS of components #445

Open
domoritz opened this issue Aug 11, 2019 · 22 comments
Open

Refactor CSS of components #445

domoritz opened this issue Aug 11, 2019 · 22 comments

Comments

@domoritz
Copy link
Member

Right now we use a lot of custom classes. Instead, we should have a single class for a component (e.g. help-modal) and then use rules that are for objects nested below (e.g. .help-modal p a).

@nik72619c
Copy link

I'll give it a try :)

@domoritz
Copy link
Member Author

Sweet, thanks.

@nik72619c
Copy link

I'm not quite able to get clarity on refactoring css here...any examples of how is it to be refactored @domoritz ?

@domoritz
Copy link
Member Author

Sure! The main problems I see is that we have a lot of repetition across files, we don’t have a good way of name spacing properties right now (the styles in one file could affect other components), and we don’t use consistent style for similar components.

I think we want to reduce the number of classes and make the existing ones more meaningful and reusable. Similarly, we should use css variables for colors. We could use tachyons as a css framework to simplify our style.

Working on this will mean that you have to look across files and refactor.

Does this make sense?

@rav1kantsingh
Copy link
Contributor

I would like to help on this issue. Is there any branch for this issue?

@domoritz
Copy link
Member Author

domoritz commented Dec 5, 2019

Welcome to Vega! Not yet. Feel free to send a pull request. I recommend starting with one component and then going from there.

@rav1kantsingh
Copy link
Contributor

As you mentioned to use CSS variables for colors. should I define color variables on top of same CSS file or in some other file?

@domoritz
Copy link
Member Author

domoritz commented Dec 5, 2019

Let's define them in the main CSS file. If you work on variables first, let's replace all colors at once.

@PunitLodha
Copy link
Contributor

Hey @domoritz . I am new to Open Source. I would like to work on this issue. Should i replace the classes by the tachyons classes?

@domoritz
Copy link
Member Author

That's a lot of work. Maybe let's start by just refactoring the CSS. Right now, we have a lot of duplications and few clear separations. We can switch to a framework once you have had some exposure to our current style.

@rav1kantsingh
Copy link
Contributor

I am also open to this work. Please let me know how can I help.

@domoritz
Copy link
Member Author

Have a look at the code and send PRs to fix issues you see.

@PunitLodha
Copy link
Contributor

I have sent a PR. Please review it. If its good, then i shall start working on the other components

@PunitLodha
Copy link
Contributor

Should we start using BEM for structuring the CSS?

@domoritz
Copy link
Member Author

I'm leaving this decision to you as long as we overall increase consistency.

@adrijshikhar
Copy link

adrijshikhar commented Feb 18, 2020

@domoritz is this issue still open? I would like to take this.

@domoritz
Copy link
Member Author

Yep, the CSS can always be improved.

@adrijshikhar
Copy link

can you give me an example of how you want it to be?

@domoritz
Copy link
Member Author

Not really. Just look through the code and notice how we could clean up the CSS. I know, this issue isn't very concrete.

@ashu8912
Copy link
Member

Hey @domoritz I am working on this
also are we planning to bring theming feature to the editor in future?
coz than we will have to make css refactors while keeping that in mind.

@domoritz
Copy link
Member Author

I don't think theming is a priority.

@ashu8912
Copy link
Member

ashu8912 commented Feb 22, 2020

ok for now all the modal have distorted css working on fixing that within one pr

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

No branches or pull requests

6 participants