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

Primer CSS 13.0.2 #911

Merged
merged 34 commits into from
Oct 7, 2019
Merged

Primer CSS 13.0.2 #911

merged 34 commits into from
Oct 7, 2019

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Sep 25, 2019

Primer CSS Patch Release

Version: 📦 13.0.2
Approximate release date: 📆 26/09/2019

📝 Documentation

🏠 Internal


Ship checklist

  • Update the version field in package.json
  • Update CHANGELOG.md
  • Merge this PR and create a new release
  • Test the release candidate version with github/github
  • Update github/github

For more details, see RELEASING.md.

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Sep 25, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://primer-css-git-release-1302.primer.now.sh

@vercel vercel bot temporarily deployed to staging September 25, 2019 22:34 Inactive
@vercel vercel bot temporarily deployed to staging September 25, 2019 22:35 Inactive
Use inline octicon in branch name example
@vercel vercel bot temporarily deployed to staging September 25, 2019 22:35 Inactive
Report needless disables in CI, bump stylelint, tidy scripts
@shawnbot
Copy link
Contributor

Ooh, this is an interesting failure! It looks like the changes in #900 removed selectors and the new deprecations test noticed that they weren't listed. How do you want to resolve this, @simurai?

@simurai
Copy link
Contributor Author

simurai commented Sep 26, 2019

Ooh, this is an interesting failure! It looks like the changes in #900 removed selectors and the new deprecations test noticed that they weren't listed.

Haaa.. 🤔

  1. Add these selectors to deprecations.js. Is that getting messy, quickly? I guess these selectors are not really deprecated, and could be used again in the future.
  2. Is it possible to ignore pseudo selectors like :first-child? Might be tricky with a selector like .SelectMenu-item + .SelectMenu-item. Somehow ignore if the selector contains any of (space), :, ::, >, [ or so. Maybe ignore if it has any special character except the first . period.

@shawnbot
Copy link
Contributor

shawnbot commented Sep 26, 2019

I'm really reluctant to ignore pseudo-selectors because it's possible that they're relied upon (for instance, on github.com, where it's really hard to know for sure). I think if we're going to add them to the deprecations list we'll need to make a new major release out of this branch. I'm sorry, but I'm a stickler on semver, and I think that if we're going to remove selectors we have to be really clear about that in our versioning. (Removing pseudo-selectors actually seems more important to document than class selectors in a way, because they're harder to test.)

I think our options are:

  1. Patch this branch with a PR that restores the removed selectors.
  2. Create a new 14.0.0 release branch off this one and close this PR.
  3. Revert More SelectMenu improvements #900 and merge a new PR without the removals.

My preference would be for 1) just to avoid the "overhead" of another major release, but if bringing back the removed selectors is going to be tricky, let's roll with 14.0.0. Reverting PRs can be messy and I like to avoid it whenever possible. Anyone else have opinions, @primer/ds-core ?

@simurai
Copy link
Contributor Author

simurai commented Sep 27, 2019

I'm sorry, but I'm a stickler on semver

😂 I think that's great. It should prevent accidental bugs to creep in. 👍

1. Patch this branch with a PR that restores the removed selectors.

A dirty workaround to fix the border bug AND not remove any existing selectors would be to add the selectors back, without actually using them. 🙈 I'll make a PR to test that.

2. Create a new 14.0.0 release branch off this one and close this PR.

Might be fine, but we just released 13.0.0. The only "notable change" so far would be that the border is fixed when filtering in the SelectMenu. So it feels a bit too early for another major.

3. Revert #900 and merge a new PR without the removals.

It would block using the SelectMenu that has a filter, like https://github.com/github/github/pull/116285. Well, not really block, but just make it look less polished. It's not that important to replace the branch picker select menu as soon possible, so maybe fine to wait till next major release.

So in summary, let's see how dirty option 1 feels, otherwise do option 3 and wait a bit for the next major release.

Related thought: Since it can be quite costly to quickly iterate on new components, maybe we need a place where components can first be tested for a while? And only move them to Primer CSS once we feel "fully backed". Some places that come to mind:

  • /hacks. Although it would not really be a "hack".
  • /components. Ohh.. wait, we wanna get rid of them. 😆
  • A new folder called /incubator or similar.

I'll make a proposal for that last point ☝️ .

@simurai
Copy link
Contributor Author

simurai commented Sep 27, 2019

@shawnbot Here the PR that adds the selectors back: #916

It feels a bit like cheating. 😬 It might be ok to do that in urgent situations. Not sure if fixing the SelectMenu is urgent enough.

@simurai
Copy link
Contributor Author

simurai commented Oct 1, 2019

Reverted #900 and opened a new PR #922 for next major release.

@simurai
Copy link
Contributor Author

simurai commented Oct 3, 2019

@shawnbot How about we release this as is and not wait for "Move Keyboard Shortcuts #904"? I moved it to 14.0.0.

I think that also allows us to skip updating github/github. 🙌

@simurai simurai marked this pull request as ready for review October 3, 2019 01:23
@simurai simurai requested a review from shawnbot October 3, 2019 01:23
Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM @simurai! 🚀

@simurai simurai merged commit da8ee54 into master Oct 7, 2019
@shawnbot shawnbot deleted the release-13.0.2 branch October 22, 2019 22:46
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 this pull request may close these issues.

4 participants