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

Add support for various inline styles #526

Merged
merged 10 commits into from
Feb 4, 2021

Conversation

tneotia
Copy link
Collaborator

@tneotia tneotia commented Feb 2, 2021

Fixes #516, implements everything that was WIP in #228.

Implements direction, display, font-feature-settings, font-family, font-style, font-size, font-weight, line-height, text-decoration, text-decoration-line, text-decoration-color, text-decoration-style, and text-shadow support based on CSS guidelines.

Added Display.NONE to the existing Display enums to allow style="display: none;"

Preview:

image

Code for above example:

<p style="direction: rtl;">This text should go from right to left</p>
<p style="display: none;">This text should not show up</p>
<p style="font-feature-settings: smcp 1;">This text's lower case letters should be small capital</p>
<p style="font-family: times;">This text should be in Times New Roman</p>
<p style="font-style: italic; font-family: monospace;">This text should be italic and in monospace</p>
<p style="font-weight: bold;">This text should be bold</p>
<p style="line-height: 3">This is going to be a really long text element just to test line height which I have set in the style of this paragraph to be three times the height of this paragraph's font size.</p>
<p style="line-height: 100%">This is going to be a really long text element just to display the default line height which is the same as the height of this paragraph's font size multiplied by 1.2 as per CSS spec.</p>
<p style="text-shadow: 2 2, 0 0 5px #DE6721;">This text should have a really weird text shadow</p>
<p style="text-decoration: underline overline line-through #DE6721 wavy;">This text should have some fancy text decorations</p>

This is going to need some pretty rigorous testing imo, there's a lot of different variables to consider when creating the inline style implementations so I want to make sure as many edge-cases are covered as possible.

@tneotia tneotia changed the title Add support for the font-size inline style Add support for direction, display, font-family, font-style, font-size, and font-weight inline styles Feb 2, 2021
@tneotia tneotia changed the title Add support for direction, display, font-family, font-style, font-size, and font-weight inline styles Add support for direction, display, font-feature-settings, font-family, font-style, font-size, and font-weight inline styles Feb 3, 2021
@tneotia tneotia changed the title Add support for direction, display, font-feature-settings, font-family, font-style, font-size, and font-weight inline styles Add support for various inline styles Feb 3, 2021
Copy link
Collaborator

@erickok erickok left a comment

Choose a reason for hiding this comment

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

Good work. Just slightly confusing variable naming here for me.

lib/src/css_parser.dart Show resolved Hide resolved
}
}
}
fontFeatures.toSet().toList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't do anything right? (it creates a new list, bu you don't assign it to anything).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It removes duplicates from the list. Kind of odd, but it works nicely and is super concise.

Copy link
Collaborator

@erickok erickok Feb 3, 2021

Choose a reason for hiding this comment

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

I thought that's what you wanted, but it doesn't, because the list is not modified in-place.

    test("toSet_inline", () async {
      final list = ["one", "two", "one"];
      list.toSet().toList();
      expect(list, ["one", "two"]);
    });

Will fail:

Expected: ['one', 'two']
  Actual: ['one', 'two', 'one']

But this works of course:

    test("toSet_inline", () async {
      final list = ["one", "two", "one"];
      expect(list.toSet().toList(), ["one", "two"]);
    });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought it was modified in place. I will fix this as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}
}
}
shadow.toSet().toList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here - this has no effect right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It removes duplicates from the list here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

lib/style.dart Outdated

const FontSize(this.size);
const FontSize(this.size, this.units);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would have preferred to have the units as optional param, with '' as default.

Alternatively, use named constructors for perc, px and rm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have made it optional... don't know what I was thinking I was probably a little tired 😅 I'll do the same for LineHeight.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@erickok erickok merged commit 8bd4e0b into Sub6Resources:master Feb 4, 2021
@erickok erickok mentioned this pull request Feb 8, 2021
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.

font-size not working
2 participants