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

Edit docs for typography attributes #6454

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

nickmcintyre
Copy link
Member

@nickmcintyre nickmcintyre commented Oct 3, 2023

Addresses #6449

Changes:
I edited the inline documentation for functions that set typography attributes to bring them more in line with the style guide.

@davepagurek @dkessner @MsQCompSci @limzykenneth @Qianqianye @dhowe, @paulaxisabel, @SarveshLimaye, @SkylerW99, @hannahvy, @BamaCharanChhandogi, @singshris, @EarthIsHeavin, @skbhagat0502 I'd love to incorporate any feedback you may have!

textAscent() and textDescent() still confuse me, so I'd appreciate your guidance there.

src/typography/attributes.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dhowe dhowe left a comment

Choose a reason for hiding this comment

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

I noticed one typo (see comment), but otherwise looks good, thanks!

@limzykenneth
Copy link
Member

In terms of textAscent and textDescent I'm unsure which aspects confused you so do clarify if you can but looking at the example for those two you created, I think you mostly got it. However I will probably suggest changing the examples to use instead of vertical line, to use horizontal lines instead, explained along with the following diagram

A diagram showing terms to do with letter height and positioning on the baseline. Alternative terms are italicised.

Where for textAscent we draw the baseline and the top ascent line only while for textDescent we draw baseline and the bottom descent line.

@dkessner
Copy link

Overall readability looks good. I like the suggestion from @limzykenneth to use horizontal lines in the diagrams to explain textAscent and textDescent.

@nickmcintyre
Copy link
Member Author

@limzykenneth @dkessner I restored edited versions of the original examples for textAscent() and textDescent(). The originals were broken using the default system font on macOS, so I loaded Inconsolata for consistency.

Needing to multiply by scalar messes with my intuition. It feels like textAscent() and textDescent() should do this internally. We can table that for another time.

@dkessner @MsQCompSci here's a staging version of the docs for this pull request. Don't wander too far off the Reference pages or stuff will break–patched just enough of the site to get this working.

Please let me know if you'd like to see further revisions or if we should request a merge.

@dhowe
Copy link
Contributor

dhowe commented Oct 17, 2023

Is there a problem with the image for textBounds()
Also, wondering if we should explain what 'tight bounds' means ?

@dkessner
Copy link

I'm not sure what "scalar" is supposed to be doing in this example. It's definitely confusing, and should be unnecessary based on my understanding of what textAscent() returns (the height of the 'd').

I looked at the Processing example, which has the same scalar. But weirdly, the example doesn't work when I run it in Processing on Mac (the line is in the wrong place). It does, however, work when I set the scalar to 1.0.
https://processing.org/reference/textAscent_.html

@MsQCompSci
Copy link

  • I wonder if there is a simpler example for textWidth, textAscent, textDescent. Younger users might find it difficult to understand what it is being used for in the examples included here.

@nickmcintyre
Copy link
Member Author

@dhowe revisions for the font.textBounds() documentation are part of #6453. You can preview them here.

@dkessner @MsQCompSci thanks for looking into this. Agreed that this should be simpler and will give the examples another go this evening.

@nickmcintyre
Copy link
Member Author

@dkessner @MsQCompSci the staging version of the reference for this pull request has new examples. Please let me know if you have suggestions or would like to see further simplifications.

I've never taught these functions to beginners. They are precision tools for more advanced use cases. That said, textAscent() and textDescent() do seem overly complex. I'm probably missing some context for their design.

@dhowe
Copy link
Contributor

dhowe commented Oct 25, 2023

Indeed, the fontScale argument does seem less than intuitive. At very least we were trying to match the API for processing at the time, but I'm not sure if there were other factors at play. Taking a very quick glance at the code below, it would seem that this argument could be omitted (and the renderer's current textSize used instead), and thus could be marked as optional. Has anyone tested this?

image

_textAscent(fontSize) {

@nickmcintyre
Copy link
Member Author

@dhowe thanks for looking into this. I just tested the following changes with the textAscent() example:

// ...
textSize(24);
// ...
let a = textAscent(24);
// ...
textSize(48);
// ..
a = textAscent(48);
// ...

Passing the current font size to textAscent(), as in textAscent(24), didn't affect the ascender's height. It's still necessary to multiply textAscent(24) * fontScale. Calling font._textAscent(24) works the same way. It might be worth starting a separate discussion about textAscent() and textDescent().

@dhowe @Qianqianye @limzykenneth @davepagurek if these changes look good to you, please go ahead an merge this pull request.

@dhowe
Copy link
Contributor

dhowe commented Nov 2, 2023

Passing the current font size to textAscent(), as in textAscent(24), didn't affect the ascender's height. It's still necessary to multiply textAscent(24) * fontScale. Calling font._textAscent(24) works the same way. It might be worth starting a separate discussion about textAscent() and textDescent().

If it were possible to include an appropriate fontScale as a default, this would seem to be more intuitive, though we'd then have to weigh it against a) backward compatibility and b) api-sync with processing

@nickmcintyre
Copy link
Member Author

@davepagurek @limzykenneth @Qianqianye if these changes look good to you, please go ahead and merge them.

@Qianqianye
Copy link
Contributor

Thank you all for reviewing. I will merge this PR for now and we can continue the conversation about fontScale.

@Qianqianye Qianqianye merged commit bd42ed1 into processing:main Nov 13, 2023
2 checks passed
@nickmcintyre nickmcintyre deleted the sod-typography-attributes branch May 6, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants