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

Dynamic fonts #3698

Merged
merged 33 commits into from Feb 21, 2020
Merged

Dynamic fonts #3698

merged 33 commits into from Feb 21, 2020

Conversation

samelhusseini
Copy link
Contributor

@samelhusseini samelhusseini commented Feb 19, 2020

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Fixes #2793
Partially addresses #2694

Proposed Changes

Use theme options in the renderer.
Support custom font styles either by extending the renderer and overriding getDefaultFontStyle_ or by setting the font style in a theme.
Support the ability for flyout buttons to declare in CSS their font styles, and measure the height when the flyout opens.
Computing text height during initializes and anytime a theme changes. Font metrics include: height and baseline, baseline is used when FIELD_TEXT_BASELINE_CENTER is set to false (geras and Edge / IE).
Added debugger element to show text baseline of field labels.
Text constants are all computed now, no more browser specific heights / offsets.

Reason for Changes

Dynamic font sizes.

Test Coverage

Tested all blocks in the playground in all supported browsers.
Tested both FIELD_TEXT_BASELINE_CENTER true and false.

Tested on:

Documentation

Additional Information

Screen Shot 2020-02-18 at 5 44 08 PM

Screen Shot 2020-02-18 at 5 43 55 PM

@rachel-fenichel
Copy link
Collaborator

@BeksOmega fyi

tests/playground.html Show resolved Hide resolved
core/theme.js Show resolved Hide resolved
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Random subset of comments--I'm not done reading.

core/field.js Show resolved Hide resolved
core/field.js Show resolved Hide resolved
core/field_checkbox.js Show resolved Hide resolved
core/field_checkbox.js Show resolved Hide resolved
core/renderers/common/constants.js Show resolved Hide resolved
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Overall I really like it! I didn't see any code problems, but I haven't pulled it to test it yet.

One thing I'm curious about is the changes to isDirty. Was this just for code consistency or did you run into problems? Either way I'm cool with removing it where you did.

Otherwise it was just little doc things. Thanks for your work on this :D

core/field.js Show resolved Hide resolved
core/field_colour.js Outdated Show resolved Hide resolved
core/field_dropdown.js Show resolved Hide resolved
@@ -273,7 +273,7 @@ Blockly.utils.dom.getTextWidth = function(textElement) {
* This method requires that we know the text element's font family and size in
* advance. Similar to `getTextWidth`, we cache the width we compute.
* @param {!Element} textElement An SVG 'text' element.
* @param {number} fontSize The font size to use.
* @param {string} fontSize The font size to use.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a breaking change, so it should be marked for doc updates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The change here is just whether you expect the 'pt' on the font size to be added here or by the calling code, right? What's the advantage of changing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flyout button dynamically reads from CSS what the font size should be, which generates the font size in pixels. I needed to change this in order to support both pt for fields and px for flyout buttons.

As I think about this, there's a way to do this without making it a breaking change. I'll just add another util method that uses pixels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm struggling to come up with the other method name, how do we feel about getFastTextWidthWithSizeString.

core/field.js Show resolved Hide resolved
core/renderers/common/renderer.js Show resolved Hide resolved
core/theme.js Outdated Show resolved Hide resolved
core/theme.js Outdated Show resolved Hide resolved
core/flyout_base.js Outdated Show resolved Hide resolved
tests/playground.html Show resolved Hide resolved
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Non-field files look solid (pending some changes we just discussed).

@BeksOmega
Copy link
Collaborator

I pulled down the branch and did some testing and it looks really good! I did find a smattering of small issues though so I'm just going to smatter them at you :P

Multiline text field is a bit broken.
Size-CodeField

Geras inline inputs are kinda weird. But I don't think this is a bug.
Size-InlineInputs

Checkbox needs to re-render when the character is changed (this one is probably mine hehe)
Size-CheckCharacter

Dropdown carrots act differently in geras and zelos. In geras they match text in zelos they do not. Not sure if this is a bug, or which way it goes if it is. Might want to ask devs what they want.
Size-Dropdown1
Size-Dropdown2

Warnings are a bit broken. They do resize pretty well if you close & reopen them though.
Size-Warnings

Comments (block & workspace) don't reflect text size. Not sure if they should.
Size-BlockComment

Zelos flyout buttons don't reflect text size.
Size-ZelosButtons

I think the text category flyout label should automatically match the text size since it only overrides the font family, which it is not doing. But I'm not sure, this could be intended.
Size-FlyoutLabel

Label gaps act a bit oddly.
Size-FlyoutLables2

      <label text="test" web-class="customButton"></label>
      <sep gap="8"></sep>
      <label text="test" web-class="customButton"></label>
      <label text="test" web-class="customButton"></label>
      <spe gap="32"></spe>
      <label text="test"></label>
      <sep gap="8"></sep>
      <label text="test"></label>
      <label text="test"></label>
.customButton > text {
      font-family: Impact !important;
      font-size: 40px !important;
    }

I expect both <sep gap="8"></sep> gaps to be the same. But I'll admit this looks nice.

And I think that #2694 should be left open, as applying a class directly to the field still doesn't quite work.
Size-BlockLabel

If you want me to create separate issues for any of these I'm totally cool with that! (┏-‿ ◕)┏

Thanks for all your hard work!

@samelhusseini
Copy link
Contributor Author

samelhusseini commented Feb 20, 2020

Multiline text field is a bit broken.

Will fix.

Geras inline inputs are kinda weird. But I don't think this is a bug.

Yeah, that's expected. Thrasos doesn't have that problem as it vertically aligns everything.

Checkbox needs to re-render when the character is changed (this one is probably mine hehe)

I think a forceRerender would fix this in setCheckCharacter, but I'm going to leave this one out as it's unrelated (Feel free to file an issue for this).

Dropdown carrots act differently in geras and zelos. In geras they match text in zelos they do not. Not sure if this is a bug, or which way it goes if it is. Might want to ask devs what they want.

True, the background here is that zelos uses SVG and we don't resize the SVG based on the text font size. Whereas dropdowns are text in geras and are resized when the font changes.
Could look into this as a separate change.

Warnings are a bit broken. They do resize pretty well if you close & reopen them though.

This one's a little tricky as icons don't have render loops.

  • We could add render loops, and re-render on a theme change.
  • reset visibility for warnings that are open. (ie: set to false, and back to true, which would dispose and recreate the bubble)
  • close all warnings when we need to re-render the workspace.

Comments (block & workspace) don't reflect text size. Not sure if they should.

Also simple to make them reflect the size, currently renderers aren't applying it's font properties to .blocklyCommentTextarea.
Will have a think about this one, but will keep out of this change.

Zelos flyout buttons don't reflect text size.

That's by design, I added an override in the playground in order to test custom CSS.

I think the text category flyout label should automatically match the text size since it only overrides the font family, which it is not doing. But I'm not sure, this could be intended.

Similar to comments, we can have a think about applying the renderer font properties to that as well.

Label gaps act a bit oddly.

The flyout gaps are the same, its just that text has different height and in turn gaps around its legible text depending on the font size. Not sure what can be done here.

@samelhusseini
Copy link
Contributor Author

Thanks all for reviewing and for your feedback! Apologies that this ended up being on the larger end of changes that I usually like. But hey dynamic fonts woo!

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.

None yet

5 participants