-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
Drop Legacy Edge support. #31972
Drop Legacy Edge support. #31972
Conversation
1648731
to
68fb51a
Compare
Split from #30986. I think we are going to leave any further JS code cleanup for later. @twbs/css-review please make sure I didn't remove something that's still needed 🙂 |
68fb51a
to
671541f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great for CSS! Stoked to drop so much for even the form range component. Crazy how much those pseudo elements add.
671541f
to
d2c13e0
Compare
site/content/docs/5.0/migration.md
Outdated
@@ -9,6 +9,10 @@ toc: true | |||
|
|||
## v5.0.0-alpha3 | |||
|
|||
### Browser support | |||
|
|||
- Dropped support for Microsoft Legacy Edge. See [here](#browser-support-1) for the previous browser support changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there is any other way to do this so that it's foolproof; now it's fragile and will break if we add another Browser support
heading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another naming nit-pick. 'Microsoft Legacy Edge' should be 'Microsoft Edge Legacy'. (that's Microsoft's official name)
REF: https://support.microsoft.com/en-us/microsoft-edge/what-is-microsoft-edge-legacy-3e779e55-4c55-08e6-ecc8-2333768c0fb0
https://support.microsoft.com/en-us/microsoft-edge/what-s-the-difference-between-the-new-microsoft-edge-and-microsoft-edge-legacy-a01258e5-8c05-7bbb-bed2-c65bec0eb126
Still spotted some Edge references in the md files ( |
I intentionally kept those, should I remove them?
…On Thu, Oct 29, 2020, 20:11 Martijn Cuppens ***@***.***> wrote:
Still spotted some Edge references in the md files (browsers-devices.md
for example). CSS wise it looks ok.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#31972 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNI7UI7O4NAKWRFUG5TSNGV65ANCNFSM4S7PZJVQ>
.
|
Yeah, this seems to be incorrect now:
Just spotted this could also be removed: bootstrap/scss/forms/_floating-labels.scss Lines 66 to 85 in d2c13e0
|
All right, I'll update the docs tomorrow too.
…On Thu, Oct 29, 2020, 20:42 Martijn Cuppens ***@***.***> wrote:
Yeah, this seems to be incorrect now:
https://github.com/twbs/bootstrap/blob/d2c13e0f3187ed00f62fb3905cc34b1ffd7d3010/site/content/docs/5.0/getting-started/browsers-devices.md#L11
Just spotted this could also be removed:
https://github.com/twbs/bootstrap/blob/d2c13e0f3187ed00f62fb3905cc34b1ffd7d3010/scss/forms/_floating-labels.scss#L66-L85
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#31972 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNNI5GLCQZVF3V42NDTSNGZSNANCNFSM4S7PZJVQ>
.
|
d2c13e0
to
c6c736a
Compare
PR updated. The only thing I'm not sure about is this:
|
Sidenote here about our philosophy: shouldn't we use progressive enhancement as a basis, to ensure nothing's broken on unsupported browsers? I'm thinking of our new floating labels—as far as I remember they were barely unusable no Edge before I added a few things— but there might be more cases. Could be as easy as wrapping our code in |
Good question @ffoodd. I think our approach needs to be more about embracing the future, and less about making the old stuff work. At least with our major version releases. If folks need to support older browsers, they should continue using v4. We should codify that maybe in these docs and update that snippet that @XhmikosR highlighted. |
c6c736a
to
c4177c0
Compare
I think this is ready for a final review. I made #32057 for the polyfills, but I could also move that change here. I assume we all stick with the decision to drop Legacy Edge support, right? Unfortunately, there's no sweet spot between size/redundant code and progressive enhancement AFAICT; we either keep some dead code or forget about progressive enhancement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 happy to see those polyfills gone
08a94ba
to
047bfca
Compare
Merged the polyfills removal to this PR so that we have everything in one place. I think the only thing left is this: #31972 (comment)
|
I'd say remove that sentence entirely @XhmikosR. |
047bfca
to
eb06ad1
Compare
.browserslistrc
Outdated
@@ -5,7 +5,9 @@ last 1 major version | |||
not dead | |||
Chrome >= 60 | |||
Firefox >= 60 | |||
Edge >= 16 | |||
# needed since Edge still has usage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should probably say 'needed since Edge Legacy still has usage;'
.browserslistrc
Outdated
@@ -5,7 +5,9 @@ last 1 major version | |||
not dead | |||
Chrome >= 60 | |||
Firefox >= 60 | |||
Edge >= 16 | |||
# needed since Edge still has usage; | |||
# 79 was the first Edgium version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Edgium' was a nickname and not an official term. I suggest it says 'Chromium' or 'Blink' instead.
site/content/docs/5.0/migration.md
Outdated
@@ -9,6 +9,10 @@ toc: true | |||
|
|||
## v5.0.0-alpha3 | |||
|
|||
### Browser support | |||
|
|||
- Dropped support for Microsoft Legacy Edge. See [here](#browser-support-1) for the previous browser support changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another naming nit-pick. 'Microsoft Legacy Edge' should be 'Microsoft Edge Legacy'. (that's Microsoft's official name)
REF: https://support.microsoft.com/en-us/microsoft-edge/what-is-microsoft-edge-legacy-3e779e55-4c55-08e6-ecc8-2333768c0fb0
https://support.microsoft.com/en-us/microsoft-edge/what-s-the-difference-between-the-new-microsoft-edge-and-microsoft-edge-legacy-a01258e5-8c05-7bbb-bed2-c65bec0eb126
This allows us to move forward without being held back. Microsoft already replaces the Legacy Edge with the new one on supported Windows versions.
4130ee6
to
df7df9a
Compare
Needs thorough review in case something is wrong, for example some of the vendor prefixes.
Also, it seems we can drop more code like the Polyfills, but I need some help with that.Preview: https://deploy-preview-31972--twbs-bootstrap.netlify.app/
Closes #32017 along the way