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

resolves #320 set CSS variables for font size, font family and line height #530

Merged
merged 4 commits into from
Mar 5, 2022

Conversation

ggrossetie
Copy link
Member

Use --asciidoc-font-family, --asciidoc-font-size and --asciidoc-line-height variables.

Fix #320
Fix #365

Use `--asciidoc-font-family`, `--asciidoc-font-size` and `--asciidoc-line-height` variables.
@ggrossetie ggrossetie requested a review from danyill March 3, 2022 16:12
@danyill
Copy link
Contributor

danyill commented Mar 4, 2022

I will take a look this weekend :-)

Copy link
Contributor

@danyill danyill left a comment

Choose a reason for hiding this comment

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

Thank you for the opportunity to review this.

I have made a few comments but I don't think having full control over line height is worth much effort as a feature and making this comprehensive could be some work...

I think we should remove the duplicate line-height definition but anything else I'm not sure about.

media/asciidoctor-editor.css Outdated Show resolved Hide resolved
@ggrossetie
Copy link
Member Author

I think we should remove the duplicate line-height definition but anything else I'm not sure about.

I removed the duplicate line-height and I'm now using calc to compute the value relative to var(--asciidoc-line-height, 1.6).
For instance, line-height: 1; is equivalent to line-height: calc(var(--asciidoc-line-height, 1.6) * 0.625); when --asciidoc-line-height is 1.6.

In my opinion, it gives a decent result.

@ggrossetie ggrossetie requested a review from danyill March 5, 2022 10:47
Copy link
Contributor

@danyill danyill left a comment

Choose a reason for hiding this comment

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

I can't get a preview at all at present with either the default or the editor stylesheet. Some of it is due to incorrect line-height definitions but there is something else which I am struggling to find. I'll look for a little bit more (I don't have good tools or approach for this and my 👀 fail me).

media/asciidoctor-default.css Outdated Show resolved Hide resolved
media/asciidoctor-default.css Outdated Show resolved Hide resolved
media/asciidoctor-default.css Outdated Show resolved Hide resolved
@danyill
Copy link
Contributor

danyill commented Mar 5, 2022

In my opinion, it gives a decent result.

Very nice. I like the use of calc.

Although I can't find it, somehow both CSS files are invalid and on my machine I can't receive a preview. We must have somehow slightly different environments that this is working for you or I am missing a commit (?).

image

I don't get any useful debug information on this error in the extension host logs or the webview developer tools console.

I see:

Error loading webview: Error: Could not register service workers: InvalidStateError: Failed to register a ServiceWorker: The document is in an invalid state..
onDidChangeNotification @ workbench.desktop.main.js:787

I think this means there is a CSS definition problem somewhere as nothing else has changed but I can't find it and neither can the linter or https://jigsaw.w3.org/css-validator/validator

@ggrossetie
Copy link
Member Author

Although I can't find it, somehow both CSS files are invalid and on my machine I can't receive a preview. We must have somehow slightly different environments that this is working for you or I am missing a commit (?).

line-height was missing on a few places in asciidoctor-default.css but asciidoctor-editor.css was fine and now both are working on my end.

I don't get any useful debug information on this error in the extension host logs or the webview developer tools console.
I think this means there is a CSS definition problem somewhere as nothing else has changed but I can't find it and neither can the linter or jigsaw.w3.org/css-validator/validator

Not sure... for reference, I'm testing using the test-workspace/full.adoc file.

preview

@ggrossetie ggrossetie requested a review from danyill March 5, 2022 18:25
Copy link
Contributor

@danyill danyill left a comment

Choose a reason for hiding this comment

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

Not sure... for reference, I'm testing using the test-workspace/full.adoc file.

Somehow my node_modules were polluted and a refresh fixed it.

I have tested and I think that's a great implementation 👍

@ggrossetie ggrossetie merged commit fafc098 into asciidoctor:master Mar 5, 2022
@ggrossetie ggrossetie deleted the issue-320-font-size branch March 5, 2022 18:40
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.

Preview: Font Size and Line Height not working Font size / preview style too large and setting doesn't work
2 participants