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: inappropriate text wrapping during layout calculation #1791

Merged
merged 1 commit into from
Mar 12, 2019

Conversation

nathggns
Copy link
Contributor

In some cases, it seems there was an extra line being inserted during drop cap layout calculation on iOS, as seen below.

image (1)

I spent quite a long time thinking this was an issue in the layout engine, but eventually ruled that out when I noticed that the height of one of the words going into it was double what it needed to be. When I made the layout text elements appear, I noticed one of the words was wrapping inappropriately, which was throwing off the calculations in the layout engine.

image (2)

It seems that the defined width on the iOS container, which I believe was added to cancel an iOS optimisation from preventing onLayout from being called, was causing React Native to intermittently attempt to wrap certain words before it gave up. Removing this defined width prevents the inappropriate wrapping, and it also seems we don't need that defined width to ensure onLayout is called.

image (3)

In terms of the cause of this inappropriate wrapping, I'm not entirely sure what it is, but it seems it could be connected to facebook/react-native#18258

@jmars you may well be particularly interested in this as you may know more about why the defined width was necessary in the first place.

@jimhunty
Copy link
Contributor

Should this be captured in a test?

@nathggns
Copy link
Contributor Author

@jimhunty as tests don't run onLayout, it's hard to test this package in any meaningful way that would expose this issue. In theory we could run a snapshot test on the styles, but it's not going to test any meaningful part of the package unfortunately.

@nathggns nathggns force-pushed the fix/drop-cap-calc branch from b7694e2 to 0f6add8 Compare March 11, 2019 14:42
@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.393% when pulling 0f6add8 on fix/drop-cap-calc into fc46e55 on master.

Copy link
Contributor

@jimhunty jimhunty left a comment

Choose a reason for hiding this comment

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

Sounds like a test approach for this should be considered in future.

@jimhunty jimhunty merged commit bf836b8 into master Mar 12, 2019
@jimhunty jimhunty deleted the fix/drop-cap-calc branch March 12, 2019 09:57
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.

3 participants