-
Notifications
You must be signed in to change notification settings - Fork 151
feat: truncated text initial commit web component #7397
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
feat: truncated text initial commit web component #7397
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7397 +/- ##
==========================================
- Coverage 84.49% 84.46% -0.04%
==========================================
Files 425 427 +2
Lines 17288 17453 +165
Branches 4589 4625 +36
==========================================
+ Hits 14608 14742 +134
- Misses 2680 2711 +31
🚀 New features to boost your workflow:
|
packages/ibm-products-web-components/src/components/string-formatter/string-formatter.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, but I know @davidmenendez was considering something like useOverflowString
, but maybe that’s more for utility format.
I do think this should be classified as a utility component. But I’d love for us to consider the naming if there are better ways to describe its purpose.
(Maybe @RichKummer has thoughts?)
Also we should label this a feat
not a chore
:D
# FullPageError | ||
|
||
Display a full-page error when the requested page is unavailable to the user. | ||
This is typically caused by issues with the requested URL or access permissions. | ||
Errors caused by server connectivity issues are not covered in this guideline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update this. :)
ah, the framework agonistic utility, let me try that parallel. sounds like a good idea to me. |
|
i have updated this pr, and the changes include. name change to tests will be fixed soon |
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.stories.ts
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.mdx
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.mdx
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.mdx
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.mdx
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.ts
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.ts
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.ts
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.scss
Show resolved
Hide resolved
…text/truncated-text.stories.ts Co-authored-by: elysia <[email protected]>
…text/truncated-text.mdx Co-authored-by: elysia <[email protected]>
…text/truncated-text.mdx Co-authored-by: elysia <[email protected]>
…text/truncated-text.mdx Co-authored-by: elysia <[email protected]>
the align right seems to be adding some unnecessary styles, it could be adding some rtl direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small comments.
Also wonder if we’Ll need to move some of the styles to @carbon/ibm-products-styles
once we add the React version.
inline-size: 1em; | ||
inset-block-start: 0; | ||
inset-inline-start: calc(-1em - 0.2em); | ||
padding-inline-start: 0.2em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we’re using em
instead of rem
here? If we’re using these specific numbers over tokens, we should also comment on how we derived these; e.g. avoid magic numbers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i intended em
particularly in this case as this utility (or component) will be used in various html text elements, that has varying font sizes, so setting padding-inline-start: 0.2em
would set the tiny padding between ...
and the text to 0.2em
and it scales as per the context (parent font size) its used in. so a tiny text will have tiny padding, and larger text will have larger padding.
there will be cases where the font sizes are modified in an application.
cases where custom typography mixins could be applied to certain elements.
in such cases, if we use rem, it will always be derived from the root, but em derive the units from the parent.
added the comment and removed padding as we are not using the gradient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wonder if ch
would be better alternative then? But can do that when we move styles to @carbon/ibm-products-styles
for the React version.
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually updating to approve — the comments I have are pretty minor and I think can be considered non-blocking. We can definitely update when we update the React implementation too.
packages/ibm-products-web-components/examples/truncated-text/src/styles.scss
Outdated
Show resolved
Hide resolved
packages/ibm-products-web-components/src/components/truncated-text/truncated-text.scss
Outdated
Show resolved
Hide resolved
f0462d9
Closes #7542
Closes #7541
adds truncated text in web components
What did you change?
added new web component
How did you test and verify your work?
setup basic test, test coverage of at least 80% will be covered in #7541