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

Allow textAlign(CENTER, CENTER) with max width but no max height #5933

Closed
1 of 17 tasks
davepagurek opened this issue Jan 4, 2023 · 11 comments · Fixed by #6060
Closed
1 of 17 tasks

Allow textAlign(CENTER, CENTER) with max width but no max height #5933

davepagurek opened this issue Jan 4, 2023 · 11 comments · Fixed by #6060

Comments

@davepagurek
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build Process
  • Unit Testing
  • Internalization
  • Friendly Errors
  • Other (specify if possible)

p5.js version

1.5.0

Web browser and version

Firefox 108.0

Operating System

MacOS 12.5.1

Steps to reproduce this

In older versions of p5 (e.g. 1.4.2), one could use textAlign(CENTER, CENTER) and then call text('something', x, y, w) without specifying a height parameter after. This used to center the text vertically about the y coordinate specified, without clipping any of the lines of text.

In 1.5.0 onwards, this results in a warning and no text displaying:

textAlign(*, CENTER) requires x, y, width and height 

Live: https://editor.p5js.org/davepagurek/sketches/2a9gQGPCX (uncomment the v1.4.2 script tag in index.html and comment out the 1.5.0 one to see the prior behaviour.)

Some p5 examples use this older form (e.g. https://p5js.org/reference/#/p5.SoundRecorder, the "Done! Tap to play and download" text) and no longer correctly display text. We can update these examples, but it might also be more futureproof to handle the no-clipping case along with the clipping case (which was broken in 1.4.2, fixed in #5787.)

@Prateek462003
Copy link
Contributor

I would like to work on this issue

@Qianqianye
Copy link
Contributor

Sounds good @Prateek462003 . Please go ahead and submit a pull request. Let us know if you have any questions. Thank you.

@dfg2-teacher
Copy link

I am having an issue with the text align method as well. Don't know if it should be here or its own issue. My issue is that when centered vertically the text above the y coordinate does not show up in the sketch. If its not centered vertically then the full text will show up.
image

@dhowe
Copy link
Contributor

dhowe commented Jan 24, 2023

@Qianqianye have the recent fixes to text-handling been made live ?

@davepagurek
Copy link
Contributor Author

davepagurek commented Jan 24, 2023

@dhowe I think this issue's fix #5880 isn't released just yet -- @dfg2-teacher I think your issue will be fixed by that in the next p5 release 🙂

@sawaisinghh
Copy link
Contributor

sawaisinghh commented Mar 8, 2023

Hi @davepagurek,
I think, we can modify the implementation of the textAlign function to include a check for the CENTER parameter and the absence of a height parameter. Like, we can add a condition:

if (vertical === CENTER && typeof h === 'undefined') { h = this._textLeading * this._textSize; }

@davepagurek
Copy link
Contributor Author

Thanks @sawaisinghh! Would you be interested in making a PR with the change and some screenshots of how it looks using the new code?

@Qianqianye
Copy link
Contributor

This might related to the issue #6056

@sawaisinghh
Copy link
Contributor

@davepagurek I made a PR, please review it.

@sawaisinghh
Copy link
Contributor

@Qianqianye yes, that issue were related to this. and I solved it in this pull request #6060

@dhowe
Copy link
Contributor

dhowe commented Mar 11, 2023

See my review in the PR here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants