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

Add layerset documentation & improve documentation #82

Merged
merged 12 commits into from Nov 24, 2021

Conversation

fredpi
Copy link
Member

@fredpi fredpi commented Nov 22, 2021

This PR adds support for layerset documentation which I stated as a goal for the v3 release (#75).

I also updated the whole documentation style to better align with the way documentation is rendered in Xcode 13.

With the new documentation style, an exemplary symbol documentation now looks as follows:

doc

Notably, when the user types the symbol name, they will now only see the documentation summary in the editor, i. e. the symbol preview. It might be suitable to add a note whether localizations or layersets are available. What do you think? @Stevenmagdy @knothed

Simple summary:

simple

Extended summary:

extended

In this PR, I shortened the availability descriptions of localizations and layerset iOS 15.0, macOS ..., tvOS ..., watchOS ... to just iOS 15.0. With this, some information is removed, but I think that's okay for the sake of better readability.

@Stevenmagdy I noticed that there are still some TODOs in the Symbols Generator project. Are those still relevant? If true, it would be better to track them in a GitHub issue instead of the code. Would you mind removing the TODOs and creating GitHub issues if needed? You may use this branch and PR; I won't commit further until you reviewed and commented the PR.

Also I think that the behavior introduced by the use of the localizationsOfAllVersions(of:) is incorrect: E. g., the legacy a is described as offering multiple localizations, but Apple only guarantees that those are available for the successor (character). Therefore, providing the same localization information for both the legacy and the current symbol name isn't correct, right? If you agree, would you mind changing this behavior?

@StevenSorial
Copy link
Member

StevenSorial commented Nov 22, 2021

@fredpi Thank you for the PR, this is nice cleanup of the docs

It might be suitable to add a note whether localizations or layersets are available. What do you think? @Stevenmagdy @knothed
Extended summary:

I think the extended summary looks better and is not too long, I also suggest adding ", ⚠️ Restricted" for restricted symbols.

In this PR, I shortened the availability descriptions of localizations and layerset iOS 15.0, macOS ..., tvOS ..., watchOS ... to just iOS 15.0. With this, some information is removed, but I think that's okay for the sake of better readability.

I think it might be better to use the version (i.e 2.0) like SFSymbols.app shows in the sidebar. what do you think?

@Stevenmagdy I noticed that there are still some TODOs in the Symbols Generator project.

You could remove the one in Types.swift, it was already implemented in #80. I will create an issue for the other one.

Also I think that the behavior introduced by the use of the localizationsOfAllVersions(of:) is incorrect

Actually, it's correct, old names of symbols inherit the same features/look/localizations of the new names. Apple didn't state this officially, but this is how it behaves. The only case that is not supported is when giving an old name an explicit localization, which we don't support anyway (we should). here is a full example:

This is running in an Arabic simulator

Screen Shot 2021-11-22 at 9 29 53 PM

I think we should also do the same with layersets for old names since old names support it too.

@fredpi
Copy link
Member Author

fredpi commented Nov 22, 2021

@Stevenmagdy Thanks for the quick review!

I think the extended summary looks better and is not too long, I also suggest adding ", ⚠️ Restricted" for restricted symbols.

Good idea, I will implement this 👍

I think it might be better to use the version (i.e 2.0) like SFSymbols.app shows in the sidebar. what do you think?

Hmm, I'm not very happy with this. Although this versioning is also used in the SF Symbols app, I think many users won't understand what this versioning means, while it's immediately clear from the iOS version – at least that's what I suspect. Or is there a reason to assume the opposite?

You could remove the one in Types.swift, it was already implemented in #80. I will create an issue for the other one.

Okay, then I will remove both TODOs in the code.

I think we should also do the same with layersets for old names since old names support it too.

Thanks for the detailed example 👍 I will implement the changes as you suggested.

Regarding explicit access to a localized symbol – while this works, I don't believe that such a use is encouraged by Apple – or is there a place in the docs that mentions this kind of use? But before we proceed with the discussion: We should probably not discuss this here, but rather in a separate issue :)

@fredpi
Copy link
Member Author

fredpi commented Nov 22, 2021

@Stevenmagdy I have had a look at the second TODO and changed it a bit in a commit.

I now think that it isn't necessary to create an issue for it because it's not urgent to remove the deprecated variable and it is also nothing that we should do anytime soon (so a issue would be only distracting).

@StevenSorial
Copy link
Member

Hmm, I'm not very happy with this. Although this versioning is also used in the SF Symbols app, I think many users won't understand what this versioning means, while it's immediately clear from the iOS version – at least that's what I suspect. Or is there a reason to assume the opposite?

I agree and I also don't like it, but I think it's the best option since this is how Apple versions the symbols. also keep in mind that not every platform follows the same release timeline, for example, version 2021 and 2021.1 both refer to macOS 12.0 but for iOS its 15.0 and 15.1 respectively.

Regarding explicit access to a localized symbol – while this works, I don't believe that such a use is encouraged by Apple – or is there a place in the docs that mentions this kind of use.

I'm 100% sure that it was explicitly mentioned in one of the WWDC videos but yeah let's discuss it in a separate issue.

@fredpi
Copy link
Member Author

fredpi commented Nov 22, 2021

@Stevenmagdy

Hmm, I'm not very happy with this. Although this versioning is also used in the SF Symbols app, I think many users won't understand what this versioning means, while it's immediately clear from the iOS version – at least that's what I suspect. Or is there a reason to assume the opposite?

I agree and I also don't like it, but I think it's the best option since this is how Apple versions the symbols. also keep in mind that not every platform follows the same release timeline, for example, version 2021 and 2021.1 both refer to macOS 12.0 but for iOS its 15.0 and 15.1 respectively.

With the last commits, I changed to both specifying the SF Symbols version and the iOS version. I also tried only specifying the symbols versions, but to me it seemed like that wouldn't be a good idea (the user won't have an intuition what the symbol version approximately means). What do you think about the idea of specifying both?

I also implemented the other suggested changes.

@StevenSorial
Copy link
Member

What do you think about the idea of specifying both?

It's definitely better, but don't you think the user might think that the localization/layerset is only supported on iOS?

I tried including all of them, since its the clearest option, it does not look too bad:

Screen Shot 2021-11-23 at 1 37 53 AM

I think it's better to include all of the platforms just to avoid confusion, even though it's a longer description.

@fredpi
Copy link
Member Author

fredpi commented Nov 23, 2021

@Stevenmagdy I implemented both your proposals (always printing out layerset docs & printing the availability for all OS types).

@StevenSorial
Copy link
Member

LGTM. I added one possible improvement but it's ok if you decide not to.

@StevenSorial
Copy link
Member

Thanks @fredpi

@StevenSorial StevenSorial merged commit 95a4c17 into stable Nov 24, 2021
@StevenSorial StevenSorial deleted the work/layerset-support branch November 24, 2021 00:58
@fredpi fredpi mentioned this pull request Nov 26, 2021
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.

None yet

2 participants