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

Fix #357: TextNode: render text when dimensions are specified #390

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Fix #357: TextNode: render text when dimensions are specified #390

merged 1 commit into from
Mar 12, 2019

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Mar 7, 2019

screenshot from 2019-03-07 15-19-37

A few questions:

  • Should the fixed text be cropped to its dimensions?
  • Should I add a test for this? If so, how?

@bryphe
Copy link
Member

bryphe commented Mar 12, 2019

This looks great, @romgrk ! Thanks for fixing this - I like the way you refactored the code 👍

Regarding your questions:

Should the fixed text be cropped to its dimensions?

I think the behavior you show in your screenshot is reasonable - especially since @Akin909 implemented text overflow in #392 , and we have the ability to add overflow(Hidden) on the parent.

Should I add a test for this? If so, how?

Really appreciate you thinking about this! 💯 Unfortunately there is a blocking bug on our CI machine - i tracked in #400 - the Azure CI environments don't support creating an OpenGL context (at the moment). Since the test would need to touch the draw method, which makes OpenGL calls, testing that portion of the stack isn't doable until we get #400 sorted out. But I think what you have right now is reasonable.

I'll bring in this as it looks good overall - thanks for the help, @romgrk ! Really appreciate the contribution.

@bryphe bryphe merged commit 562ecf8 into revery-ui:master Mar 12, 2019
@bryphe bryphe mentioned this pull request Mar 12, 2019
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