8000 feat(text-base): Add Span vertical-align support by bundyo · Pull Request #8257 · NativeScript/NativeScript · GitHub
[go: up one dir, main page]
Skip to content

feat(text-base): Add Span vertical-align support #8257

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 6, 2020
Merged

feat(text-base): Add Span vertical-align support #8257

merged 10 commits into from
Jun 6, 2020

Conversation

bundyo
Copy link
Contributor
@bundyo bundyo commented Jan 18, 2020

PR Checklist

What is the current behavior?

Currently there's no way to align vertically the Spans in a FormattedString

What is the new behavior?

Spans in FormattedString support the vertical-align CSS property.

Implements #4795.

@bundyo bundyo requested a review from vakrilov January 18, 2020 11:11
@bundyo bundyo self-assigned this Jan 18, 2020
@cla-bot cla-bot bot added the cla: yes label Jan 18, 2020
vakrilov
vakrilov approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor
@vakrilov vakrilov left a comment

Choose a reason for hiding this comment

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

LGTM!
Minor refactoring suggested

< 8000 /div>
@fgutteridge
Copy link

Any updates here, @bundyo @vakrilov?

@bundyo
Copy link
Contributor Author
bundyo commented Mar 18, 2020

I'll try to get back to this when I have time.

@dosomder
Copy link
Contributor

Looking forward to this feature. Thanks @bundyo 👍

@cla-bot
Copy link
cla-bot bot commented Apr 1, 2020

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Alexander Vakrilov.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

tarunama
tarunama reviewed Apr 15, 2020
View reviewed changes
@NathanWalker NathanWalker force-pushed the bundev/add-span-vertical-align branch from aa4c5ee to 05b0355 Compare June 6, 2020 20:17
@NathanWalker
Copy link
Contributor

Thanks @bundyo for this - I went ahead and refactored based on all suggestions with some minor cleanup and going to get this merged and out 👍

@NathanWalker NathanWalker merged commit faa0181 into master Jun 6, 2020
@NathanWalker NathanWalker deleted the bundev/add-span-vertical-align branch June 6, 2020 21:03
@dosomder dosomder mentioned this pull request Jun 7, 2020
Merged
5 tasks
@felixkrautschuk felixkrautschuk mentioned this pull request Jul 22, 2020
Closed
NathanWalker
NathanWalker reviewed Aug 6, 2020
View reviewed changes
nativescript-core/ui/text-base/text-base.android.ts
}
[lineHeightProperty.setNative](value: number) {
this.nativeTextViewProtected.setLineSpacing(value * layout.getDisplayDensity(), 1);
const fontHeight = this.nativeTextViewProtected.getPaint().getFontMetricsInt(null);
this.nativeTextViewProtected.setLineSpacing(Math.max(value - fontHeight, 0) * layout.getDisplayDensity(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

@bundyo was this intended here and tested as far as you know? We are seeing some oddities on android with line height related to this change.

Copy link
Contributor Author
@bundyo bundyo Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, unfortunately the line height in Android wasn't really line height, but instead the space between the two lines, so I changed it to follow what a line height is on web. I communicated then, that this was a breaking change and maybe better left for a major version.

NathanWalker pushed a commit that referenced this pull request Aug 7, 2020
@bundyo @NathanWalker
feat(text-base): Add Span vertical-align support (#8257)
8535b6d
NathanWalker pushed a commit that referenced this pull request Aug 7, 2020
@bundyo @NathanWalker
feat(text-base): Add Span vertical-align support (#8257)
9cc8e02
tarunama pushed a commit to tarunama/NativeScript that referenced this pull request Sep 9, 2020
@bundyo @tarunama
feat(text-base): Add Span vertical-align support (NativeScript#8257)
b51ed19
@CatchABus CatchABus mentioned this pull request Oct 31, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanWalker NathanWalker NathanWalker left review comments

@tarunama tarunama tarunama left review comments

@vakrilov vakrilov vakrilov approved these changes

Assignees

@bundyo bundyo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bundyo @fgutteridge @dosomder @NathanWalker @vakrilov @tarunama @vtrifonov
2927
0