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

white space implementation #120

Conversation

swingingtom
Copy link
Collaborator

This PR is more a complete resolution for #109 than #117
But is also more complex. It implements white-space property with normal,pre-wrap & 'pre-line' based on https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model/Whitespace and https://developer.mozilla.org/en-US/docs/Web/CSS/white-space

@swingingtom
Copy link
Collaborator Author

Whitespace implementation could be externalized because it is currently split on different components during different phases:

  1. Text during parseParams (here)
    Alter the provided text content according to white-space
  2. Text after glyphDimensions (here)
    Define the linebreak property according to white-space
  3. Block(inlineManager) during computeInlinesPosition (compute lines dimensions here and here)
    Collapse remaining whitechars according to white-space

It also externalize computeWidth with a simple left to right length (here)

@swingingtom
Copy link
Collaborator Author

Examples are still showing white-space (glitch) because defaults was set to 'pre-wrap' which I interpreted as being the implicitely implemention.

Examples are also modified to suggest resolution for nested blocks & tutorial result
Settings defaults to 'pre-line' fix all examples at once, and may have less effects for existing users/projects than 'normal'. The less risky is 'pre-wrap' I guess.

@felixmariotto
Copy link
Owner

Hi @swingingtom, thanks a lot for your work !

I think we will go for this PR over #117, because alignContent: "right" is basically broken otherwise (I don't know how I didn't notice this before btw..).

I would like to discuss about what should be the default. If we keep 'pre-wrap' as the default our problem is not solved, because people will still have wrapping issues whose fix will be burried in the doc (which they don't want to read). If we go for 'pre-line' however we would fix the wrapping issues, and be very close to the CSS default 'normal' while working around the important difference that our users don't use <br> for carriage return. So IMO we should make 'pre-line' the default, what do you think ?

Also I sent you a PM on discourse.

@swingingtom
Copy link
Collaborator Author

I think we will go for this PR over #117, because alignContent: "right" is basically broken otherwise (I don't know how I didn't notice this before btw..).

Perhaps it didn't exist before "kerning" implementation. It changed width to use xadvance which tends to be a bit bigger than width.

So IMO we should make 'pre-line' the default, what do you think ?

Same goes for me. I didn't set it because 'pre-wrap' was the displayed behaviour before I start implementing.

Also, I would like to split all the whitespace logic into another file. But Im not sure where to place it. Any suggestions ?

@swingingtom
Copy link
Collaborator Author

'pre-line' has been set as default.
Whitespace implementation has been externalised to 'utils/Whitespace.js' file. But Im still open for moving it to a much accurate location is someone have suggestions.

@felixmariotto felixmariotto merged commit 2f7b2c6 into felixmariotto:master Jan 18, 2022
@felixmariotto
Copy link
Owner

felixmariotto commented Jan 18, 2022

Thanks a lot for your work on this @swingingtom, it's really clean 👌
I published as 6.0.3

@swingingtom
Copy link
Collaborator Author

@felixmariotto Thanks. Im glad to participate.
I had in mind to delay this merge, in order to ease #122 without conflicts. I should have told you before.

@felixmariotto
Copy link
Owner

Ah my bad... Sorry

@GoncaloBastos
Copy link

No worries, with a little work I was able to merge it all together :) The main challenge was with the InlineManager, so @swingingtom and @felixmariotto make sure you double check that part before accepting #122.

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