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.

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

feat(vwc-text): 1st draft to support font faces #942

Merged
merged 31 commits into from
Jul 13, 2021
Merged

feat(vwc-text): 1st draft to support font faces #942

merged 31 commits into from
Jul 13, 2021

Conversation

yinonov
Copy link
Contributor

@yinonov yinonov commented Jul 6, 2021 •
edited
Loading

Affected scopes

  • vwc-text

    • provisions all vivid font faces
    • unit tests
    • ui tests
    • documentation

  • vvd-design-tokens

    • updated style dictionary version
    • restructured typography renderes
    • new renderer to autogenerate typescript enum of supported font faces.

image

@github-actions
Copy link

github-actions bot commented Jul 6, 2021

🚀

Latest successful build of the PR deployed here.

🚀

@yinonov yinonov added Consumers Awaiting This feature is awaited for by one or more of our consumers ready-to-merge Type: Feature labels Jul 9, 2021
YonatanKra
YonatanKra reviewed Jul 11, 2021
View reviewed changes
ui-tests/tests/vwc-text/index.js
@@ -0,0 +1,32 @@
import '@vonage/vwc-text';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be in the components/vwc-text folder. What makes this file a utility? Will it be used someplace else?
In addition, I can't see a reason it should import @vonage/vwc-text. If it is used in the vwc-text tests, it already imports the vwc-text, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a new exposed API
note https://vonage.slack.com/archives/C013F0YKH99/p1625584237238900

it is actually an (good?) idea to later integrate it within typography in components...

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean typography tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. I mean we can use this component within other components that reflect styled text. but just throwing it off the air. not sure about its benefits

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind leaving it inside the text for now?

YonatanKra
YonatanKra requested changes Jul 11, 2021
View reviewed changes
Copy link
Contributor

@YonatanKra YonatanKra left a comment

Choose a reason for hiding this comment

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

Please remove ui-tests/diff.png and ui-tests/tmpScreenshot.png. They are not relevant anymore.

@tveinfeld
Copy link
Contributor

Adding some context

tveinfeld
tveinfeld reviewed Jul 11, 2021
View reviewed changes
Copy link
Contributor

@tveinfeld tveinfeld left a comment

Choose a reason for hiding this comment

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

general question: if we can add these preset font combinations to our "context" (as classes), should we add a dedicated component for it?

YonatanKra
YonatanKra previously approved these changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@YonatanKra YonatanKra left a comment

Choose a reason for hiding this comment

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

I approve. Just see if you can get the vwc-text test utils into the vwc-text as they are not general test utils but specific to vwc-text...

YonatanKra
YonatanKra previously approved these changes Jul 13, 2021
View reviewed changes
@sonarcloud
Copy link

sonarcloud bot commented Jul 13, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yinonov yinonov merged commit 5f69fc8 into master Jul 13, 2021
@yinonov yinonov deleted the viv-622 branch July 13, 2021 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tveinfeld tveinfeld tveinfeld left review comments

@YonatanKra YonatanKra YonatanKra left review comments

Assignees
No one assigned
Labels
Consumers Awaiting This feature is awaited for by one or more of our consumers ready-to-merge Type: Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yinonov @tveinfeld @YonatanKra