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

CSS is all over the place #41

Closed
zouhir opened this issue Jan 14, 2018 · 23 comments
Closed

CSS is all over the place #41

zouhir opened this issue Jan 14, 2018 · 23 comments
Labels

Comments

@zouhir
Copy link
Owner

zouhir commented Jan 14, 2018

The client side app need some organisation, CSS wise at first.

current bad practice:
We have styles split in components folders and shared style folder

better practice:
clean the shared style folder and keep only:

  • shared mixins

  • shared grid

  • shared colour variables

  • keep each component style next to it.

@Lakston
Copy link

Lakston commented Jan 15, 2018

I might be able to look into that, another addition to consider later for css consistency would be to add a .csscomb config file to ensure that the proper attributes order is respected

@zouhir
Copy link
Owner Author

zouhir commented Jan 15, 2018

ugh! I would love to see some good CSS organisation happening! even if not everything is finished now, some good structure for us to carry on with and follow would be awesome!!!!!!!!!!! 🙏

@15blex
Copy link

15blex commented Jan 15, 2018

I would be glad to help with that too! @zouhir @Lakston
I've inspected the scss a bit alredy, can't understand the usage of a flexbox grid instead of a true grid with display: grid. That would bring the usage and needs of mixins near to 0 imho.
Another usefull thing could be postcss with autoprefixer and cssnano (i guess there's alredy a csscomb plugin too for postcss).

@Lakston
Copy link

Lakston commented Jan 15, 2018

I took a look too and tbh it wasn't a big mess, I though it would be all over the place but it looks pretty well organised already. I'll have to dig deeper but at the moment the only file that could be removed from the bundle that I've found was backgrounds.scss where only the .bee class is used and should be moved to it's component file so the backgrounds one can be deleted.

As for grids, a discussion about the browser coverage should be done first regarding IE's partial support.

Autoprefixer is already part of the bundling process (see /src/client/webpack.config.js) from what I've read.

@15blex
Copy link

15blex commented Jan 15, 2018

I don't belive is a mess either, i noticed just some sparse colors that could be put in variables.

On caniuse it says that the partial support for grids in IE is because it supports the old spec (autoprefixer should take care of it i guess), but I never had the chance to test it. Will try later if works as expected or not

My bad for autoprefixer, didn't saw it in the config 🤦‍♂️

I'll try to see if I notice something else later after work!

@zouhir
Copy link
Owner Author

zouhir commented Jan 16, 2018

Hey!! @15blex and @Lakston super thanks to both of you.

I think CSS new Grid thing is awesome for 2 reasons:

1- our audience are web developers, so I would not expect big percentage running a browser that does not support CSS grid (CSS Grid is in latest 2 of all major desktop browsers on Can I Use)
2- It will allow us to change the responsiveness of items easily, and utilise every monitor's landscape fully better

I think we should go forward with it!

I'm happy to open those 3 issues:

1- add all colors to variables folder
2- add all font sizes to varibales folder
3- grid support instead of flex based grid system

@15blex
Copy link

15blex commented Jan 17, 2018

Nice! I'll make a PR asap.

@zouhir
Copy link
Owner Author

zouhir commented Jan 17, 2018

There's now an issue #70 and they've actually suggested adding themes, I think keeping that in mind while making the CSS refactoring is a good idea.

I'll break this into a bunch of smaller issues, so we can work workout stepping on each other toes!

@15blex
Copy link

15blex commented Jan 17, 2018

Good point on #70!
Basically we need to define an object shape and a naming convention for the theme,
something like theme-name{ primary-color, secondary-color, ... }.

@zouhir
Copy link
Owner Author

zouhir commented Jan 20, 2018

Opened an issue to start a better process this weekend #76

@zouhir
Copy link
Owner Author

zouhir commented Jan 20, 2018

Linking CSS grid issue #77

@equinusocio
Copy link

equinusocio commented Jan 24, 2018

What about CSS Custom properties/mixins w/o postcss?

@15blex
Copy link

15blex commented Jan 24, 2018

I guess there should be no problem, they seems to be quite supported.
See https://caniuse.com/#feat=css-variables

With custom props the themes may become very interactive and selectable at runtime.
See https://codepen.io/chriscoyier/pen/ybgNKP

What do you think @zouhir?

@zouhir
Copy link
Owner Author

zouhir commented Jan 24, 2018

since we are targeting developers (Jarvis is a dev tool) who are most likely on latest 2 releases of browsers, I think we can use whatever supported in latest 2 as long as it brings good value!

May I learn more about the benefit of that @equinusocio and @15blex ?

Also, if you want to agree on sometime to talk CSS in Jarvis and themeing, I have a little discord channel and happy to chat https://discord.gg/XjA6Wy

@TimoStaudinger
Copy link
Contributor

Note that CSS variables are not supported in IE, which (unfortunately) is still fairly widely used at least in enterprise environments these days. Maybe worth considering.

@equinusocio
Copy link

equinusocio commented Jan 24, 2018

@timosta But jarvis is not something that is developed by his consumers, it is targeting developers. Developers can still develop for IE or any other browser, and run jarvis on supported browser.

@TimoStaudinger
Copy link
Contributor

TimoStaudinger commented Jan 24, 2018

That's absolutely correct. All I'm saying is that I've been working with companies in the past that officially allow IE (and only IE) on their machines, including for their devs.

I don't know whether or not this is Jarvis' target audience and worth consideration. Just saying that we may want give it a thought.

Edit: Looks like CSS Grid isn't really supported by IE either, so if we go with that, my point is probably moot.

@equinusocio
Copy link

equinusocio commented Jan 24, 2018

@zouhir WIth CSS custom properties, as said by @15blex, you can manipulate them runtime. You can get or set css custom properties that are applied.

// Get custom property value
document.body.getPropertyValue('--foo-bar');

// Get custom property value
document.body.setProperty('--foo-bar', newValue);

You can define simple UI components that expose css custom properties that you can theming at the document root level or by js.

Just an example:

html {
  --widget-background: pink;
}

.SpecialWidget {
  --widget-background: cyan;
}
<style>
  .Widget {
   --widgetBackground: var(--widget-background, red);

    background-color: var(--widgetBackground);
  }
</style>
<div class="Widget">
   <!-- Widget component content -->
</div>

@zouhir
Copy link
Owner Author

zouhir commented Jan 24, 2018

CSS variables might make our life easier when supporting themes, at the moment I am using Sass and CSS Modules and they might certainly minimize the code amount even for people who making themes.

Also, a big win in modern css: if we want to choose something like CSS Grid that will make the scale & number of widget shown in a monitor and the responsiveness of them a 100 times better I am happy to support only latest major browsers.

But I get @timosta point of view, and we should not execlude people who have corporate restrictions. I'd check:

1- Does VSCode run version of browser that supports variables and grid?
2- Does Atom do so too?

If they do, we are going to make extensions, so if you have 5 years old browser you can use Atom or VS Code.

@equinusocio
Copy link

My point of view is that IE 11 will be fairly widely used until developers support it. Or we can just wait a Microsoft move that will close the already not supported ie 11.

Anyway, you can also just set the default theme on older browsers. 100% fallback compatibility.

@zouhir
Copy link
Owner Author

zouhir commented Jan 24, 2018

I think I am sold on that, I do believe so many developers are using the latest and I hope by adding support to editors we not excluding people!

I have started a WIP PR here, #77 I am happy for either your feedback or ideas or give you push access or start new one if you have time, I am open if you would like to contribute with more ideas! no pressure tho!

@zouhir zouhir closed this as completed Feb 26, 2018
@equinusocio
Copy link

equinusocio commented Feb 26, 2018

@zouhir Sorry i totally missed the last message. I will check the PR.

I think that we can define custom properties inside each component root selector and decouple them from exposed ones. This approach is called "CSS API". If you want, you can check my article about that

@zouhir
Copy link
Owner Author

zouhir commented Mar 20, 2018

great article @equinusocio ! that totally make sense and thank you for sharing!

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

No branches or pull requests

5 participants