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

+semver:major [SIL.WritingSystems] Add UI font role #1389

Merged
merged 9 commits into from
Feb 18, 2025
Merged

Conversation

imnasnainaec
Copy link
Contributor

@imnasnainaec imnasnainaec commented Feb 11, 2025

Resolves #1212 (implementing #1212 (comment))


This change is Reviewable

@imnasnainaec

This comment was marked as resolved.

Copy link

github-actions bot commented Feb 11, 2025

Palaso Tests

     4 files       4 suites   16m 20s ⏱️
 4 902 tests  4 674 ✅ 228 💤 0 ❌
15 957 runs  15 273 ✅ 684 💤 0 ❌

Results for commit ff71933.

♻️ This comment has been updated with latest results.

@imnasnainaec imnasnainaec changed the title [SIL.WritingSystems] Add UI font role +semver:major [SIL.WritingSystems] Add UI font role Feb 11, 2025
@imnasnainaec imnasnainaec marked this pull request as ready for review February 11, 2025 20:03
@imnasnainaec imnasnainaec marked this pull request as draft February 11, 2025 20:09
@imnasnainaec
Copy link
Contributor Author

@ermshiperete For the roundtrip tests to work with XML files that have a font without any type specified, I thought it would make sense to change the code to default to FontRoles.None rather than FontRoles.Default. However, I'm not sure if there are usages of the library that would reasonably expect or rely on (e.g.) all write-outs some type specified on every font.

@ermshiperete
Copy link
Member

I don't know the answer to that question and I don't know where that class is used. You might want to ask on Slack.

@imnasnainaec
Copy link
Contributor Author

I don't know the answer to that question and I don't know where that class is used. You might want to ask on Slack.

No responses in #commonlibraries; should I ask again in LT Software's #public or LT's #general?

@imnasnainaec imnasnainaec marked this pull request as ready for review February 17, 2025 17:46
@ermshiperete
Copy link
Member

No responses in #commonlibraries; should I ask again in LT Software's #public or LT's #general?

Everyone who cares should watch #commonlibraries; otherwise it's documented as breaking change in the changelog, so I think we're good.

Copy link
Contributor Author

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)

@imnasnainaec imnasnainaec enabled auto-merge (squash) February 18, 2025 18:51
@imnasnainaec imnasnainaec merged commit 25540f2 into master Feb 18, 2025
8 checks passed
@imnasnainaec imnasnainaec deleted the font-roles branch February 18, 2025 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support New LDML Standard
2 participants