Skip to content

Update comment regarding default safari version#26813

Merged
sbc100 merged 1 commit intoemscripten-core:mainfrom
sbc100:safari_default
Apr 29, 2026
Merged

Update comment regarding default safari version#26813
sbc100 merged 1 commit intoemscripten-core:mainfrom
sbc100:safari_default

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented Apr 28, 2026

The minimum version was updated in #23312, but this comment was missed.

@sbc100 sbc100 requested review from dschuff and kripken April 28, 2026 23:25
Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm % comments

Comment thread src/settings.js
// Sur and iOS 14.5.
// The previous default, Safari 12.0.0 was released on September 17, 2018,
// bundled with macOS 10.14.0 Mojave.
// in MMmmVV, e.g. 160101 denotes Safari 16.1.1.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might as well make this example have the currently supported version, to avoid confusion?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean I should use the default version here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I like including an example with minior numbers here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then perhaps something like x.y.z? I ask because I read this wrong the first time, I thought the example was the current version, especially as it appears first. I think an example equal to the current version, or newer, would be less confusing.

Anyhow, maybe it's just me. lgtm either way.

Comment thread src/settings.js Outdated
The minimum version was updated in emscripten-core#23312, but this comment was missed.
@sbc100 sbc100 merged commit 730f7d1 into emscripten-core:main Apr 29, 2026
30 checks passed
@sbc100 sbc100 deleted the safari_default branch April 29, 2026 18:13
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.

2 participants