Closed wyuenho closed 7 years ago
I think you're spot on about flex and layout. I think I had a similar sort of epiphany that these just aren't the right abstractions.
I haven't had to use the other things you mentioned as much, so I have less of an opinion. I had a few headaches with typography and heading, but I think i remember my issue being controlling the display, margin, and padding, and less about styling variability.
+1 to using polished helpers.
Polished + fela bring a lot of niceness to the table, and it might be worth re-evaluating some of our previous choices in light of our recent adoptions.
I see a lot of value in
Agree with removing
I don't have a strong opinion about the rest.
Can you explain what you see in text and typography?
The ultimate goal is to have 0 global CSS styles. So if you want to style a paragraph (text) or element <cite>
you would have to do it for every single instance which doesn't enforce consistency. The other option is to extract it into a library - that's why it is in cf-ui.
The third option is the good old way - Global CSS resets. Not having a global CSS stylesheet may only buy us a few DCE'ed style properties, but we have to litter imports everywhere for these tiny things. I remain perplexed by this notion of "enforcing consistency". Nobody can and should stop a perfectly good PR that uses react-dom directly from making it in.
I mean, text doesn't even allow you to use a sementic <p>
. Not that we can't fix them by opening up a tagName or whatever, but I don't understand its reason of being. We can still get some degree of consistency by rolling some polished helper functions here.
If typography is some global thing, which it is, let's just use a global stylesheet for them, I see no reason why they need to exist as React components.
We don't write documents here, and there's no easy way to convert those elements generated by Markdown into cf-ui components.
The third option is the good old way - Global CSS resets.
I feel we should have a different and more general discussion about design principles of cf-ui that we consider immutable because otherwise we will go in circles.
To me giving green light to Fela means that we want to embrace component isolation for styles and abolish global CSS altogether. All design decisions I've done so far are strictly following this path. If I'm not sure what to do I always use that as an axiom. If there is no such principle to stand for and we can cherry-pick from both worlds, it's hard to make decisions and I feel we will not come up with something really good.
That's why I'm pushing back against selectors that are breaking the isolation and rather clone props from parents to children. Also, Robin (author of Fela) always argues against selectors. Some of them doesn't even work because of atomic classnames.
That's why I don't think that react-dom elements should be used at all since they need "Global CSS resets", otherwise they are 100% wildcards since there is no control or checks for them (we have cf-component-box as an escape hatch).
^^ To me, everything should be a component. At the beginning, the most of these components can be primitive (input, button, td or even box), later we should aim to abstract/embrace more complex ones (card, modal, form). We already have these more complex components but it's hard to use them since they are not that standardized as we would wish and designers need to give them a makeover.
I'd like to propose that we go ahead and remove the following:
cf-component-flex cf-component-layout cf-util-async cf-util-cache cf-util-logger
Let's hold off on this and discuss it this week or next week:
cf-component-heading cf-component-text cf-component-typography
The pattern I'm seeing is that we all agree on a certain sweet spot when it comes to components.
In the middle we all tend to agree on things like dropdowns and form inputs that seem to have the right level of abstraction, are easy to use with little modification, and are manageable in number.
Too large of an abstraction, and you end up with something like builder components or modal that is a pain to use and adapt. We only have a few of them, and we don't all seem to like them.
Too small of an abstraction and you end up with something like flex, text, or typography, which are highly specific, and require lots of legwork to support all required properties or components.
On one hand, adding support for all these components at the bottom of the pyramid could allow for a nice architecture, and uniform treatment which could allow better tooling and plugin adoption.
On the other hand, it could end up not being worth the investment, since whitelisting or adding every prop you'd need to make things work for all use cases would be too large, especially considering that vanilla react dom gets the job done quickly and cheaply despite being perhaps dirty.
Let me know if I've summarized this properly. Let's start a discussion somewhere to get to the bottom of this one. Can we try to quantify the costs vs benefits of each approach?
Also, here's something to think about
<li>
has a component but <p>
should be styled with global CSS?<code>
has a component but <cite>
should be styled with global CSS?Too small of an abstraction and you end up with something like flex, text, or typography, which are highly specific, and require lots of legwork to support all required properties or components.
This is a good point. If we come to a conclusion that something like <p>
should be wide open in number of supported properties we can reuse logic in cf-component-box
.
I met with design about cf-component-heading
. I'm going to submit a PR soon which splits cf-component-heading
into <PageTitle/>
, <ModalTitle/>
, and <CardTitle/>
. We can deprecate cf-component-heading
after that.
@tajo I think those are exactly the right questions. We need to determine if there is a line, and if so where it is. It doesn't seem like we've closed on that fully. Maybe <li>
and <cite>
should also get deleted, or perhaps replaced by a more use case specific component. Maybe part of the problem is that it's hard to reason about this because we don't have any <li>
or <cite>
in the wild yet.
This is a good point. If we come to a conclusion that something like
<p>
should be wide open in number of supported properties we can reuse logic in cf-component-box.
Great point. Since its implemented from the metadata in propertiesSpec, that logic should easily be re-usable.
@jwineman's work looks like a solid step in the right direction - making components based on function, not on underlying type. Headings shouldn't be grouped with other headings.
Perhaps we should strive to partition all components by function (very specific to their intended use case), and avoid making general purpose ones to adapt to use in several spots.
Why not to have both and layer? You can preserve cf-component-heading
. The only properties set by theme should be related to the font:
fontWeight
fontSize
lineHeight
and the rest will be open through props
marginTop
marginBottom
paddingTop
paddingBottom
<PageTitle/>
, <ModalTitle/>
, and <CardTitle/>
would wrap them with different margins and paddings. I doubt that designers will require different fontSize for <h1>
in a Card and in a Page but they may require different spacing. Or they will want us to use <h1>
for <PageTitle/>
and <h2>
for <CardTitle/>
.
If you remove cf-component-heading
you will end up with duplicating fontSizes
across multiple components and we will not have one consistent <h1>
or <h2>
.
For the specific case of the heading, I agree about keeping both.
I think it's still worth discussing if we should support all primitives, and if all non primitive components should be implemented exclusively as compositions of these primitives, which seems like the subtext of this thread.
We're making the argument to keep heading because we see how it would be used in conjunction with PageTitle and ModalTitle, and I agree.
Do we have something similar to make an argument for typography and text? Are we really confident the font for those will never change? I'm not sure I can judge how these will be used just yet.
If you remove cf-component-heading you will end up with duplicating fontSizes across multiple components and we will not have one consistent
<h1>
or<h2>
How important is this consistency? How bad is the pain this inconsistency will cause as compared to the pain of reworking highly generic components that we haven't used frequently yet in product.
Things like <p>
or <li>
don't live in a vacuum. How will you style them if they are not a component? Your only option is to use global CSS:
p {
...
}
But what if you need different styles for "info", "success", "error" texts? Will we reintroduce class names like cf-text-info
, cf-text-success
and cf-text-error
? But why? I thought we were unhappy with this approach.
Do you want to do regularly something like this:
const MyErrorText = createComponent(() => { color: 'red' }, 'p');
<MyErrorText>Text</MyErrorText>
Do you actually want to delete components for primitives so you have an excuse to use one-offs everywhere and having zero restrictions in terms of styling?
Why
<li>
has a component but<p>
should be styled with global CSS?
Good point. I have not tried using cf-component-list, so I don't have an opinion. I would suspect that's because the list elements in dropdown and pagination has or would have taken cared of most of our use cases. The only exception was when I was implementing a list of checks for dedicated certs, which cf-component-list still cannot be of use because I will need to use a definition list. To me, lists are still typographical elements, only generated from markdown occasionally. They fall into the same category as cf-component-typography.
Why
<code>
has a component but<cite>
should be styled with global CSS?
Also a good point. I would imagine <code>
can be expanded to wrap up something like codemirror should we need it. I don't remember it's being used. Check our code base and look into <ApiExample>
.
As to heading, I think one of the biggest confusion is, they are not meant to be picked by their sizes, but by what document level they should be in. They are actually semantic elements meant to be used in HTML5 outlines. This is an accessibility issue. If we don't do it this way, screen readers may have a hard time listing the content.
Visually, the top level section's <h1>
should definitely not be the same size as the inner level section's <h1>
. Since we cannot determine the level of depths the users nest their sections, therefore we cannot say there's only ever 1 size of <h1>
. Theoretically there are infinitely many sizes of <h1>
. The way we are currently using <h1-6>
in the product is all wrong.
The inner <h3>
s should actually be <h1>
, but different sizes. But they are not now because what people typically do is first pick by size, and then apply the minimal amount of style override to them. Having cf-component-heading as a first class component leads you to believe cf-ui has this problem solved, whereas it actually cannot be solved. Let's just make people style <h1-6>
anyway they want.
In addition, the only place where we have standalone, vanilla <h1-6>
is text generated by Markdown. We have to deal with them globally anyway. Whether it is an actual static stylesheet loaded from a link tag are minor details we can iron out, but there is zero reason for cf-component-heading to pretend that there are only ever 6 kinds of headers.
@wyuenho To add to Jimmy's point it also makes the decision of which heading to use up to the developer, not the designer, which is bad imo. The designers should be in charge of visual as well as semantic hierarchy, and should pick the right components for their mockups.
The development process shouldn't be picking the component with the most correct looking font size based on the mockup, which is how it goes now. There should be literally no decision, just "do I pick the PageTitle, ModalTitle, or XZYTitle for this view"
Also, let's hold off on async, cache and logger. I'd like to see more comment on them before jumping the gun. cf-ui is over-engineered, but that's not a good enough reason to remove things that already exists. There's just 1 man's opinion so far.
There's a couple overall things you should keep in mind in this discussion:
For specific topics:
Layout:
Layout shouldn't change very often and should be very consistent across screens. Pages, card, modals, etc should all look alike, their composition and content may be different, but your goal should be to remove layout design from your app's components.
cf-component-flex
: This was intended for smaller UI elements like inline forms, it was not meant to be used as higher level layout. If anything this component is just poorly named.cf-component-layout
: This should be replaced, not removed. It should be replaced by more rigid UI layout components that enforce consistency.cf-component-viewport
: If you want to use element queries instead of window queries, you should: 1. Talk to designers, 2: Replace this component with another one that does element queries.Utils:
Utilities are a good place to abstract away libraries that you use to prevent yourselves from getting too tied to them. You can actually do some really powerful things if you force yourselves to use them everywhere.
cf-util-async
: You might not always need them, but if you do intend to do things in parallel or in series they are useful. There's no maintenance cost to keeping the util, so I don't see why you'd want to remove it.cf-util-cache
: Go ahead, kill it with fire if you don't need itcf-util-logger
: This library was intended to also wrap deadorbit. You should do that and push for it to be used everywhere.Type:
Time and time again, typography is the fastest thing to go to shit in any app. It's incredibly easy to get really inconsistent with your typography. To the point that there are tools for identifying all the fonts/colors/font-sizes/line-heights/etc on a website.
You should have a limited number of styles for your fonts and they should be consistently used everywhere. If you regularly need to customize your type then something is going very wrong.
Also, typographic sets should be designed as a whole. You shouldn't have component-specific type styling. A modal or card shouldn't have a totally different heading than might appear elsewhere in the app. It should be using a specific heading from another set.
cf-component-heading
: Have these base styles and feel free to wrap them in a couple of places. Even if you only ever use the wrappers around it, this is a useful tool for tweaking the heading types everywhere. If semantics are a problem: change the API, don't get rid of it.cf-component-text
: This comes straight out of the existing UI. You shouldn't have dozens of different colors or styles for your text. If you're fighting it, then you're doing something wrong that is is designed to prevent. It's working as intended. cf-component-typography
: This is supposed to work with a reworked cf-util-markdown
, there's a react library somewhere that will convert markdown into actual react elements and you can override those elements with your own. Outside of markdown it should be pretty rare that you are manually putting together these kinds of elements. In those cases you will want to correctly match what is being rendered with markdown, sharing components is the right way to do that.Other James: Great points!
Just because something is only used in a few places doesn't mean it's not worth having standardized
The counter argument is that prematurely generalizing something before having literally any usage can lead to mis-predicting the use cases, which in turn results in lots of rework, or people avoiding using cf-ui entirely if under pressure.
I'm not proposing we never generalize early, but I'd like to acknowledge it has a cost associated with it if it doesn't pan out.
The other thing to keep in mind here is that design isn't a static target, and the UI didn't freeze in time a year ago. Design teams often no more capable than engineering teams at predicting the future.
The counter argument is that prematurely generalizing something before having literally any usage can lead to mis-predicting the use cases
Fair, but that's a bit different than what I was trying to say which was: Just because you only have 3 occurrences of X in the app doesn't mean you can't share the code. Which is a bit different than saying just because you currently only have 3 occurrences.
The other thing to keep in mind here is that design isn't a static target, and the UI didn't freeze in time a year ago. Design teams often no more capable than engineering teams at predicting the future.
Generally shared components like buttons, inputs, headings, etc. don't change very often and when they do they are changed everywhere. When we talk about things we can't predict that includes:
Not much to say about 1, but 2 & 3 should require some discussion. Those are the times when you talk to designers about what their designs mean for the UI/code (sometimes designers don't realize they've broken a pattern that exists and may change their design based on that feedback), then you figure out a way to make the change and propose it to the team.
It might seem like tedious work, but it scales better and better over time. Eventually you have a really robust framework which already supports everything you need to do, and because you've taken the time to do everything right you can make larger changes very easily.
cf-ui should push you to be rigidly consistent, it should be hard to break design consistency
We cannot achieve consistency by having something useless. Design consistency already exists in larger composite components like card, page, modal and table. Design consistency also demands consistency among smaller components after they are composed. No application code should use <Heading>
by itself.
If you are worried that cf-ui doesn't come with a full suite of components with composable styles, provide a global CSS reset and typography stylesheet for react-dom, either in a static file or some global theme, write in the readme that in order to setup cf-ui, you have to wire up these baseline styles first.
Just because something is only used in a few places doesn't mean it's not worth having standardized
Agree, I will only add that if they are only used in a few places, we should ask why. If they can be fixed, fix them, otherwise, it's a sign they are useless. If the uselessness comes from being at the wrong level of abstraction, remove it. Wrong abstraction is worse than no abstraction.
It's worthwhile to wrap libraries. It allows you to swap them out, improve upon them, fix bugs inside the wrapper in case you can't get a fix into the upstream library. There's a lot of opportunity there
Agree, and it should be done on a case by case basis. Not everything needs to be wrapped. Stable, standardized APIs like Web Platform APIs and ES6 methods need not be wrapped. In addition, in order to completely wrap a library, just preserving signatures is not enough, we need to preserve the behavior, which often amounts to reinventing half of whatever you just swapped out. It should be exceedingly rare that we'll ever need to swap out external libraries if we minimize external dependencies. Should we need to, the dep's interface BECOMES the wrapper and interface we need to preserve, if we directly depend on it. One extra facet layer doesn't alway help, but they could, so we should use our best judgement.
cf-component-layout: This should be replaced, not removed. It should be replaced by more rigid UI layout components that enforce consistency.
Agree. But it doesn't appear to be a need to have a grid system. The larger components are already styled by a conceptual grid. CSS grid systems are here to help prototyping, which should be left to the designers. Should we need one for engineering, we'll introduce it back that's done with CSS grid layout. The current implementation is very broken, and I'm not sure we'd like to maintain it.
cf-component-viewport: If you want to use element queries instead of window queries, you should: 1. Talk to designers, 2: Replace this component with another one that does element queries.
Agree. It doesn't bother me as much at this point.
cf-util-async: You might not always need them, but if you do intend to do things in parallel or in series they are useful. There's no maintenance cost to keeping the util, so I don't see why you'd want to remove it.
Agree. This one has no cost of maintenance, so I'm fine either way, tho this is the answer of one of our interview questions :P
Time and time again, typography is the fastest thing to go to shit in any app. It's incredibly easy to get really inconsistent with your typography. .....
Completely agree with these 3 paragraphs. I only add that consistency need not be achieved by React Component alone.
cf-component-heading: Have these base styles and feel free to wrap them in a couple of places. Even if you only ever use the wrappers around it, this is a useful tool for tweaking the heading types everywhere. If semantics are a problem: change the API, don't get rid of it.
Again, there's nothing to generalize, and it's not possible to generalize. We need headers, they are done in the Title components and a few other places. As long as they are consistent with the design, all we have to do is to import Title. These components also have no use for cf-component-heading, because they should all really be <h1>
, with an occasional <h2>
, but all slightly different in sizes and weight.
As to changing API, that's actually an interesting point. If we could say <Heading size={1} depth={2}>
, then we are getting somewhere. But then again, it should be exceedingly rare that you'll ever want to reach for <Heading>
in the application code, so I'm not sure it's worthwhile. Best experiment and incubate something like this in the application first, then push upstream instead.
cf-component-text: This comes straight out of the existing UI. You shouldn't have dozens of different colors or styles for your text. If you're fighting it, then you're doing something wrong that is is designed to prevent. It's working as intended.
No such Backbone component exists in the code base. They are simply CSS class rules. We can apply them on any element. Not possible with <Text>
. This is best emulated by rolling a few polished helper functions.
cf-component-typography: This is supposed to work with a reworked cf-util-markdown, there's a react library somewhere that will convert markdown into actual react elements and you can override those elements with your own. Outside of markdown it should be pretty rare that you are manually putting together these kinds of elements. In those cases you will want to correctly match what is being rendered with markdown, sharing components is the right way to do that.
This ? These are static text that never changes, why slow down the already slow generation process and consume more memory by converting them to React components? Agree that it should be really really rare that we'll ever need to use them directly. Their styles never really change anyway. CSS global typography stylesheet or theme is fire and forget. No need to maintain separate components.
Wow. A lot of things in here to read. Have opinions change at all since this thread exploded? Something we can do to move this conversation forward?
Heading was removed as a dependency from all cf-components a couple weeks ago, although @jwineman thought it was a compromise, I didn't think of it this way. I have yet found an appropriate use of it in the code base. That being said, I've recently discovered that browsers have refused to implement the HTML5 outline algorithm. There's talk about addressing this issue by introducing an <h>
element into HTML5. Regardless, there's still no reason we can't replace Heading with a global default typographic theme.
I think you've convinced me here. My main concern (which I think you share) is preventing people from using <h*>
tags outside cf-ui components so we're sure they're being styled consistently. I don't entirely understand what your proposing but it seems like it would still prevent this so I'm fine either way.
I'm going to go on a limb here and just close this issue after #330 got merged.
Feel free to re-open if you disagree but I suggest opening up a new issue for each individual component you'd like deprecated to keep discussions focused.
So after much internal discussion and listening to my inner voice, I want to formally declare I want to remove these packages, and solicit comments on these proposals:
<h1-6>
, every time they are used in a component, even if it's the same<h1>
, they may still look slightly different. There's no one size fits all component we can generalize here. Just use react-dom, and style it your way depending on context.<Box>
. Everybody hates<Flex>
and nobody wants to maintain it. Let it die.<Box>
that allows us to use flexbox. If we really need a standalone, lower level primitive that's grid system, let's reintroduce a real one done with CSS grid layout.<div>s
, not even<span>
. Let's just use polished to make some equivalent helper functions that decorates fela style objects.deadorbit
, which usesdebug
, it doesn't mean we should continue to use it. What's wrong withconsole.*
? It's only used in 2 places in cf-ui now, 1 is cf- util-cache, another is cf-util-http. Easy.Please feel free to comment on any of them. My only major gripes are heading/text/flex/layout/typography. No other set opinions here.