Skip to content

feat(core): add new text-shadow property #8991

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

Merged
merged 8 commits into from
Jan 29, 2021

Conversation

tralves
Copy link
Contributor

@tralves tralves commented Oct 28, 2020

PR Checklist

What is the current behavior?

NativeScript does not support text-shadow. This has been a requested feature (#3597, #5268).

What is the new behavior?

Adds text-shadow css property (and textShadow attribute).

It implements a subset of the text-shadow web syntax. The expected format is:

<offset-x> <offset-y> <blur-radius> <color>
e.g.
2 10 4 rgb(255, 100, 100)
2px 10px 2px rgba(10, 10, 10, 0.5)
1 1 1 #55a
2 2 2 #aaa

The reasons for this limitation are:

  • some of the default values in the web syntax are not clear (i.e., people shouldn't use them anyway);
  • It is impossible to implement multiple shadows (e.g. 1px 1px 2px red, 0 0 1em blue, 0 0 0.2em blue;);
  • parsing/tokenisation would not be trivial because there are multiple formats for colors (rba(0, 0, 0), #fff, etc.). It would be a lot of work;

I thought I should have more feedback on this PR before adding more work.

I think it PR would still need:

  • Unit tests (I didn't add them yet, but I added a demo for this feature in the apps.ui);
  • Add more syntax options (maybe? which ones?);
  • Validation/syntax errors;
  • Support for formatted strings and spans (not sure if it's even possible);
  • Support for tab headers (not sure if it's even possible);

Demo (apps.ui):

ns-text-shadows

Fixes/Implements/Closes #[Issue Number].
#3597 and part of #5268.

@cla-bot cla-bot bot added the cla: yes label Oct 28, 2020
@tralves tralves changed the title feature(ui): add new text-shadow property feat(core): add new text-shadow property Oct 28, 2020
@NathanWalker NathanWalker added this to the 8.0 milestone Nov 5, 2020
@NathanWalker NathanWalker added the ready for test TSC needs to test this and confirm against live production apps and automated test suites label Nov 7, 2020
@tralves
Copy link
Contributor Author

tralves commented Dec 2, 2020

Hi!
I see that this is on the roadmap :D. As I said this PR needs some polish. Do you have pointers or tips to what I should do to make your lives easier?

* So all code that works on what is expected to be a TextView/TextInput should use `nativeTextViewProtected` so that any third party
* components that need to have two separate components can work properly without them having to duplicate all the TextBase (and decendants) functionality
* by just overriding the nativeTextViewProtected getter.
**/
get nativeTextViewProtected() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove that comment ? It is actually an important comment

This comment was marked as abuse.

offsetY: this.nativeTextViewProtected.getShadowDy(),
color: this.nativeTextViewProtected.getShadowColor(),
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

First I dont think we need a default here. We have to be careful. Get default are slow because they make native calls. And yours even make 4 of them.
We now a default view as no shadow so not having a default value should be fine and faster.
Also you should use a const value to access native view protected instead of accessing it 4 times. If a class define it as a getter your would unnecessarly call it 4 times

@@ -188,6 +188,10 @@ export class TextBase extends TextBaseCommon {
this._setNativeText();
}

[textShadowProperty.setNative](value: TextShadow) {
this._setShadow(value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why create a _setShadow method If you call it only once?
It makes the class."bigger" for now reason

This comment was marked as abuse.

@NathanWalker NathanWalker changed the base branch from master to release/8.0.0 January 21, 2021 17:32
@NathanWalker NathanWalker merged commit 2d8063c into NativeScript:release/8.0.0 Jan 29, 2021
@farfromrefug
Copy link
Collaborator

@NathanWalker you merged this without review comments being fixed ? that PR removes things which should not have been touched here !

@rigor789
Copy link
Member

@farfromrefug it's on the release/8.0.0 branch where we are still going to do cleanups & changes. Just helps manage things together - and mainly helps resolving any conflicts early.

@farfromrefug
Copy link
Collaborator

I get it. just that PR removes comments which should remain there for everyone to understand things.
I don't see how you are going to bring them back now (except manually obviously) now that it is merged.
thanks for your work guys !

NathanWalker pushed a commit that referenced this pull request Feb 12, 2021
NathanWalker pushed a commit that referenced this pull request Feb 27, 2021
NathanWalker pushed a commit that referenced this pull request Feb 27, 2021
NathanWalker pushed a commit that referenced this pull request Mar 3, 2021
NathanWalker pushed a commit that referenced this pull request Mar 5, 2021
NathanWalker pushed a commit that referenced this pull request Apr 2, 2021
NathanWalker pushed a commit that referenced this pull request Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready for test TSC needs to test this and confirm against live production apps and automated test suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants