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

Global line-height rationale, semver strategy revision #612

Closed
alecmev opened this issue Aug 3, 2016 · 30 comments · Fixed by #615
Closed

Global line-height rationale, semver strategy revision #612

alecmev opened this issue Aug 3, 2016 · 30 comments · Fixed by #615

Comments

@alecmev
Copy link

alecmev commented Aug 3, 2016

What's the rationale behind html { line-height: 1.15 }? I thought it was supposed to be just the headings? And wouldn't such a drastic change warrant a major version bump?

@alecmev alecmev changed the title Global line-height Global line-height rationale Aug 3, 2016
@thierryk
Copy link

thierryk commented Aug 3, 2016

Please check the comments in this PR (Simplify headings normalization comment) and this issue (input font: inherit also pulls in line height). As a side note, I don't think such change warrants a major version bump.

@battaglr
Copy link

battaglr commented Aug 3, 2016

@jeremejevs, as you can see from the links that @thierryk provided, initially this was just for the headings but then we realised that the whole document needed it.

Regarding version bumps, it can be tricky since everything in CSS could be considered as breaking. We already have some guidelines for our semver strategy, but anything that could improve/extend that is more than welcome.

@alecmev
Copy link
Author

alecmev commented Aug 3, 2016

Alright, thanks for the links, Github search didn't rank them high by relevance for some reason.

About the version, as you say, almost every release of normalize.css is breaking due to the project's nature. I bet you've considered this already, but in case you haven't, why not then just relax the requirements for the major version increase? Large numbers aren't a novelty anymore, thanks to Chromium and Firefox, and this would protect people from upgrading normalize.css to seemingly backwards-compatible versions, which in fact aren't. In my case, it was 4.1.1 -> 4.2.0, where half of my markup ended up looking not the way it was designed. No big deal, caught it in development, but as with any other package on NPM, I didn't expect it to break on a minor version bump.

@battaglr
Copy link

battaglr commented Aug 4, 2016

@jeremejevs, I agree with you. I'd rather do major releases more often —even if we change the mayor version multiple times in a short period of time— than risking to break some site/app —as in your case.

Today many people trust semver enough to do a minor/patch release without giving too much thought about it. Some people even configure their dependencies to automatically update minor/patch releases. Because of that, I think that this is an important topic to review.

Let's invite @jonathantneal to the conversation. 😀

@thierryk
Copy link

thierryk commented Aug 4, 2016

As we all know, any change in a styles sheet has the potential to "break" things but does that mean we should bump the major version with each release? To me, I see a difference between "breakage" and minor visual glitch and I'd definitely put this line-height change in the latter category—after all, we changed the value from normal to a value very close to the computed value for normal in browsers.

@FagnerMartinsBrack
Copy link

As we all know, any change in a styles sheet has the potential to "break" things

If all browsers normalize.css support fixes one discrepancy that makes one style to be unnecessary anymore, then removing that style won't break anything because overrides will always have a bigger specificity.

@battaglr
Copy link

battaglr commented Aug 4, 2016

@thierryk, we agree on the "every change in CSS has the potential to break things".

I'm looking beyond this particular change, and would be great if this conversation leads to closing this issue with some kind of guideline for what we should consider a breaking change —thus, requiring a major release— in the context of this project. 😀

@jonathantneal
Copy link
Contributor

jonathantneal commented Aug 4, 2016

The argument for “every change is breaking” aka major releases is:

  1. Any element selector overrides a universal selector, potentially overriding styles found in either rule.
  2. Correcting any box model related value (like display, width, height, line-height, etc,) means changing the expected box model for the affected browser.

Previously, there existed a grey area which we used for minor changes:

  1. Any change that does not necessarily impact the box model (like appearance, color, background-color, outline, etc) is low impact.
  2. Any change that overrides a keyword default with an explicit value is low impact.

And I think we all agree on non-breaking patch changes:

  1. Any update to a comment has no effect on rendering.
  2. Any sorting of the rules that has no impact on rendering (such as sorting rules to reflect HTML Living Standard sectioning) is also fine.

At the time of fixing line-height, those of us in the discussion believed this change met the minor requirements. I’ve since come to believe there’s no such thing as a minor change here; only major and patch as I described.

I’m fine with effectively removing minor releases from the project, and updating the documentation to reflect as much. I’d be very interested in all your feedback, especially from big contributors like @thierryk and @battaglr.

@alecmev alecmev changed the title Global line-height rationale Global line-height rationale, semver strategy revision Aug 4, 2016
@thierryk
Copy link

thierryk commented Aug 4, 2016

@battaglr I agree that it is important we fine tune the definition of what constitute a minor release. And I think this specific issue is the perfect one to challenge the arguments listed by @jonathantneal because to me, as I suggested earlier, the impact of this fix should be very minimal—even though it "relates" to the box model.

  • the value we use is the same, or almost the same, as the default computed value (depending on browsers).
  • there is a very high probability that that value is overwritten by authors as they very often combo font-size/line-height font-family themselves... Or even set line-height on body and various blocks to achieve "vertical rythme".

@jeremejevs don't you set any line-height in your font declaration (if you have one) or through blocks? Would it be possible to see the impact of this change on your page?

@thierryk
Copy link

thierryk commented Aug 4, 2016

I forgot to add that I'd also challenge how we qualify a minor change:

Any change that does not necessarily impact the box model (like appearance, color, background-color, outline, etc) is low impact.

Because, as we know, setting a background color on html may paint body very differently (according to its content rather than the viewport).

May be we should open an issue dedicated to that versioning thing, no?

@alecmev
Copy link
Author

alecmev commented Aug 4, 2016

@thierryk The line-height change has affected Titillium Web the most, which I use extensively in my project. At font-size: 36px:

  • line-height: normal is 55px high
  • line-height: 1.15 is 41px high

Montserrat is affected too, but to a lesser extent (a couple of pixels). Google doesn't serve fonts with a default line-height, and I didn't have line-height set on all blocks, just some. Not sure how a normal line-height is calculated, but apparently it can be quite far from 1.15 for some fonts.

I have to say, Titillium is a pretty sloppy typeface in general (has serious issues at some thicknesses & sizes), so there shouldn't be too many like it, but still, this is worth consideration.

@thierryk
Copy link

thierryk commented Aug 5, 2016

@jeremejevs thanks for the info. Google font samples inherit a 1.2 value for line-height (from body) and adjust that to a lesser value for larger size (i.e. 1.1 for font-size:64px). These opinionated values chosen by Google overwrite the default line-height (normal) which in turns create a more consistent line-height across browsers.

In other words, the 1.15 value we introduce here may create a small visual change in a browser but—at the same time—that very change assures a consistent vertical space across browsers. Something you may not have before since you were not overwriting the default line-height.

@jonathantneal
Copy link
Contributor

@thierryk, this thread’s fine for discussing the semver strategy; this line-height issue demonstrates how it failed.

I agree with your challenge to the “grey area” that was a minor release. Non box model changes can have a significant and unexpected impact on sites, too; with real examples from normalize and sanitize being background-color on html or opacity on input in Internet Explorer. And, as we now learn, changes that overrides a keyword default with an explicit value.

@battaglr
Copy link

I think that the methodology proposed by @jonathantneal in #615 could work for us, since this is a very particular project. If it "doesn't work", we don't loose anything. 😀

@thierryk
Copy link

If we go this route, then I'd suggest 2 things:

[1] we could Keep It Simple by using a X.Y notation where X relates to some style change (not 100% safe to onboard) and Y relates to anything else (100% safe to onboard).

@battaglr
Copy link

@thierryk,

  1. I don't really see the point on dropping semver. I understand that due the nature of this project we are suggesting an uncommon usage of it —by not to modifying the MINOR part—, but we are not breaking any rule for how version numbers are assigned and incremented. Furthermore, semver is the facto versioning system: most popular package managers —npm, Bower, etc— use it and devs understand it. I would keep using semver.
  2. Great idea! We already have a CHANGELOG, but would be great if we can replicate that content on each release.

@thierryk
Copy link

@battaglr,

My point is to make people understand that this versioning is specific to how CSS works and that major versions could well be considered minor versions by some/most—for example, the explicit line-height we discussed recently.
If we do not convey that particularity, people will take major versions at face value and will be reluctant to upgrade early by fear to onboard a fix that may break their stuff.
If they understand that a non-patch release may well be considered a minor version then they will be more incline to check the release notes and adopt a new version at will.
In my opinion, it is about disassociating major versions with incompatible changes.
If we (the whole community?) agree that semver does not really work with CSS then why keep using it?

@alecmev
Copy link
Author

alecmev commented Aug 19, 2016

@thierryk

people will take major versions at face value and will be reluctant to upgrade early by fear to onboard a fix that may break their stuff

But what's wrong with that? Their stuff will indeed break (read "look not as desired"), and they will need to make changes to their CSS. That requires mental effort and time investment. When a regular library following Semver goes from 1.2.0 to 1.3.0, it's expected that any code built with 1.2.0 will keep working and producing 100% the same output, with no adjustments (unless there were nasty bugs involved, but that's beyond the point).

@thierryk
Copy link

thierryk commented Aug 19, 2016

@jeremejevs

But what's wrong with that? Their stuff will indeed break (read "look not as desired"), and they will need to make changes to their CSS.

What's wrong with that is that it is not necessary true, and that's my whole point. Simply because this is CSS and it depends more on "their CSS" than changes in releases.

When a regular library following Semver goes from 1.2.0 to 1.3.0

Yes, but normalize is not your regular library because it deals with CSS and when it comes to CSS you cannot assume much without knowing the project it applies to.

You say:

it's expected that any code built with 1.2.0 will keep working and producing 100% the same output, with no adjustments

But it's also expected that a major version bump would break things—which is not necessary the case with normalize, hence the idea that we should stop making parallels (with Semver) when there is none.

@alecmev
Copy link
Author

alecmev commented Aug 19, 2016

@thierryk

What's wrong is that it is not necessary true
...
But it's also expected that a major version bump would break things

But a breaking change is a breaking change, and it doesn't have to be a complete API revamp. When it comes to programming, it doesn't matter how exotic a method is. If it's documented, and it's being removed / its signature or behavior are being changed, then that deserves a major version bump. In such a case, close to 100% of users won't have to change a thing in "their JS" when upgrading, but it still should be a new major release. I know quite a few libraries in the React ecosystem, where most users could fast-forward through some major versions (Redux 2.0 -> 3.0 comes to mind).

Don't see why the same wouldn't apply to CSS.

@battaglr
Copy link

battaglr commented Aug 19, 2016

@thierryk, I agree with @jeremejevs in this. It's not that semver does not apply to CSS, is just that in this particular project MINOR releases will not be used anymore, since we target mostly tags that we can't predict how are going to be used, that's why I think almost all changes are breaking, not because they certainly will be breaking, but because we can't guarantee that they would not be breaking.

@thierryk
Copy link

I think almost all changes are breaking, not because they certainly will be breaking, but because we cant guarantee that they would not be breaking.

Exactly! And that makes very little sense in the world of Semver where bumps are tied to changes within the project itself without outside considerations...

In my opinion, abandoning Semver is an opportunity to discuss CSS versioning and its peculiarities.

@alecmev
Copy link
Author

alecmev commented Aug 19, 2016

But there's no choice here, really. If this is being published with npm / bower, then it must be SemVer.

@thierryk
Copy link

Unfortunately, this is another example where the rationale is more about tooling than providing the best solution for the user (who wants to be able to upgrade early and safely).
This is why I suggested earlier that the line-height change should be considered a "minor" change—but according to its true sense, not according to Semver's semantics.

@battaglr
Copy link

@thierryk,

I think that we are all trying to provide the best solution for the user, if not, we will not be discussing this, and probably doing other things with our time.

Sadly, I think that you are treating assumptions as truths.

[...] people will take major versions at face value and will be reluctant to upgrade early by fear to onboard a fix that may break their stuff.

They can read the CHANGELOG or as you suggested the release notes that are a good idea to start adding —basically, replicating the CHANGELOG. After that, they can decide if they want to upgrade or not. If they don't, they will still have a version that works for them, and not a new one that maybe could break something.

I honestly don't see how providing a new way of versioning that would be very specific for this project will help either. In my opinion that will do more harm than good. Most people already know and understand semver. If we decide to modify what MAJOR and MINOR means, we will need to make sure that is really well explained and documented to avoid confusions.

In my opinion there is nothing wrong with semver being used in a CSS project. In this particular case that we are basically just modifying the "global scope" is safer to treat every change as breaking. In projects were you have more defined scopes, you can be sure that modifying some module will not brake anything else, and the same applies if you add a new functionality.

I apologize if some of this sounds brusque; it's not the intention. 😀

@thierryk
Copy link

@battaglr,

I think that we are all trying to provide the best solution for the user

I'm not saying otherwise. We're discussing different POVs regarding something that is more complex than it seems. I'm just voicing my opinion, which happens to be different than yours—that's all.

Sadly, I think that you are treating assumptions as truths.

[...] people will take major versions at face value and will be reluctant to upgrade early by fear to onboard a fix that may break their stuff.

How's that an assumption? A major version bump means devs should be extra cautious when upgrading, and that's my problem with versioning against major/patch versions only. Because most devs will consider that each (non-patch) release includes "incompatible API changes"— when in fact some of them will be minor changes with great benefits that would justify an early adoption.

I apologize if some of this sounds brusque

No worries. We're here to find solutions. And please understand that I'm not trying to have the last word here, I'm just trying to explain my POV the best I can. To me, there is value in "tagging" some of the releases as "minor" versions (as in low-risk, not in the Semver sense since we are dealing with CSS). And that's why I said we could move away from Semver because "minor" would have a very different meaning.

@battaglr
Copy link

battaglr commented Aug 20, 2016

@thierryk,

Great that we are keeping things friendly. Sometimes my not-so-good understanding of English makes me misinterpret some things. 😅

Going back to topic: walking way from semver is not the best solution for this problem IMO. I don't think that doing all major releases is the perfect solution but I do think that is the best solution considering all the variables.

I agree that a major release means devs should be extra cautious when upgrading, but that doesn't mean they will stop upgrading, they will have to read the CHANGELOG or release notes as any responsible dev should do. If the changes bring great benefits, I'm sure that it would not stop anyone to at least try to upgrade. That said, I'm more worried about devs upgrading because they trust in a change that we consider as low-risk, but that end up having a big impact. We have perfect examples of that happening as @jonathantneal explains in #615:

[...] a change to opacity might cause inputs to disappear,
or a change to background-color might cause backgrounds to shrink.

Anyhow, I agree that we can communicate that we consider some changes to be less risky than others —while still doing a major release—, maybe by including some kind of "notice" in the release notes, i.e. "Although this is a major release is considered low-risk, meaning that {{explanation}}". That way we can encourage devs to try it with less concerns, but with caution at the same time.

I think that this is all I got to contribute to this conversation. 😀

@thierryk
Copy link

Great that we are keeping things friendly. Sometimes my not-so-good understanding of English makes me misinterpret some things.

??
Either your not-so-good english made you see things that were not there or my not-so-good english made me missed things that were there... :)

That way we can encourage devs to try it with less concerns, but with caution at the same time.

Yes, I like this idea. Because I think it is important to convey changes for which there is more reward than risk (i.e. this line-height example). As you suggested, I think we can achieve this by writing smart release notes—so people may be better equipped to make their decision.

@battaglr
Copy link

battaglr commented Aug 20, 2016

[...] your not-so-good english made you see things that were not there.

I think that this would be the case. 😝

I'm happy that we arrived to a solution that addresses the concerns of all of us. Yay!

@jonathantneal
Copy link
Contributor

Awesome dialog. I’m moving forward with the semver strategy. Every CSS change is major, and understanding the risk of any change is relative to the implementation. We can’t know all the tens of millions of existing implementations, therefore we cannot promise backwards compatibility, which means we will not have minor releases.

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

Successfully merging a pull request may close this issue.

5 participants