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

fix: pageheader subtitle truncation visibility #6551

Merged
merged 12 commits into from
Dec 13, 2024

Conversation

davidmenendez
Copy link
Contributor

Closes #6534

two issues i'd like to point out

  1. StringFormatter needs to be updated to be TypeScript
  2. StringFormatter doesn't dynamically hide the overflow text. I'm not sure if we have an alternative to StringFormatter that can display the text without making the popover always visible. would like some thoughts on this.
Screenshot 2024-12-05 at 1 22 40 PM Screenshot 2024-12-05 at 1 28 02 PM

@davidmenendez davidmenendez requested a review from a team as a code owner December 5, 2024 19:28
@davidmenendez davidmenendez requested review from makafsal and anamikaanu96 and removed request for a team December 5, 2024 19:28
Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for ibm-products-web-components ready!

Name Link
🔨 Latest commit e7a3117
🔍 Latest deploy log https://app.netlify.com/sites/ibm-products-web-components/deploys/675affcbe10e8900084dd424
😎 Deploy Preview https://deploy-preview-6551--ibm-products-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for carbon-for-ibm-products ready!

Name Link
🔨 Latest commit a0fd063
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/675bc0979c11ad0008cc31c3
😎 Deploy Preview https://deploy-preview-6551--carbon-for-ibm-products.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@davidmenendez
Copy link
Contributor Author

even using DefinitionTooltip, which is what the title does, it won't dynamically change without adding additional code to flip it from needs truncation to doesn't need truncation. the code that handles that logic in PageHeaderTitle could be moved into a hook and reused.

just wanted to get some opinions on all this because it turned out to be more complicated than i thought 😅

@davidmenendez
Copy link
Contributor Author

updated the subtitle to use DefinitionTooltip similar to title and added a new hook that re-uses the overflow logic for the title to be used in subtitle

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.05%. Comparing base (36bd4f9) to head (a0fd063).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6551      +/-   ##
==========================================
- Coverage   80.06%   80.05%   -0.01%     
==========================================
  Files         394      395       +1     
  Lines       12907    12911       +4     
  Branches     4277     4278       +1     
==========================================
+ Hits        10334    10336       +2     
- Misses       2573     2575       +2     
Components Coverage Δ
ibm-products ∅ <ø> (∅)
ibm-products-web-components ∅ <ø> (∅)

makafsal
makafsal previously approved these changes Dec 9, 2024
Copy link
Member

@makafsal makafsal left a comment

Choose a reason for hiding this comment

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

Long text in subtitle breaks styles.

Copy link
Contributor

@anamikaanu96 anamikaanu96 left a comment

Choose a reason for hiding this comment

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

Hey @davidmenendez, It looks like the styles and tooltip are breaking when we update the subtitle using the controls.
Screenshot 2024-12-09 at 2 01 12 PM

Copy link
Contributor

@amal-k-joy amal-k-joy left a comment

Choose a reason for hiding this comment

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

Change looks good. However found this behaviour , but not sure is really applicable to adopter. However , could not see this issue for title

Screen.Recording.2024-12-09.at.4.46.58.PM.mov

@makafsal makafsal self-requested a review December 9, 2024 13:36
@davidmenendez
Copy link
Contributor Author

@anamikaanu96 @makafsal @amal-k-joy thank you for catching this. i realized that my problem was that i wasn't accounting for calculating the overflow by height. i refactored my code to take that into consideration for subtitle.

@davidmenendez
Copy link
Contributor Author

i know this needs to include testing, but i wanted to get feedback on the implementation of this solution. specifically with regards to replaceWithOverflow.ts.

would love to get y'all's opinion on this!

cc: @elycheea @matthewgallo

@amal-k-joy
Copy link
Contributor

i know this needs to include testing, but i wanted to get feedback on the implementation of this solution. specifically with regards to replaceWithOverflow.ts.

would love to get y'all's opinion on this!

cc: @elycheea @matthewgallo

Solution looks good to me , since we we clamping vertically based on number of lines, its sensible to check for height overflow in case of title.
Also a thought, regarding the util naming, the functionality looks more like checking for overflow. (checkForOverflow, checkForOverflowWidth, checkForOverflowHeight )

@davidmenendez
Copy link
Contributor Author

Solution looks good to me , since we we clamping vertically based on number of lines, its sensible to check for height overflow in case of title. Also a thought, regarding the util naming, the functionality looks more like checking for overflow. (checkForOverflow, checkForOverflowWidth, checkForOverflowHeight )

to be honest i struggled a bit with naming convention here and i think it's because i got too caught up in maintaining the "normal" vs "overflow" refs, but now that i think about it those could (should) be done in the component instead of the utility.

@davidmenendez
Copy link
Contributor Author

tests have been added 👍

Copy link
Contributor

@amal-k-joy amal-k-joy left a comment

Choose a reason for hiding this comment

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

Awesome work .

@amal-k-joy amal-k-joy enabled auto-merge December 13, 2024 05:40
@amal-k-joy amal-k-joy added this pull request to the merge queue Dec 13, 2024
Merged via the queue into carbon-design-system:main with commit 26394dd Dec 13, 2024
32 checks passed
@davidmenendez davidmenendez deleted the issue-6534 branch December 13, 2024 15:37
@davidmenendez
Copy link
Contributor Author

also for #6636

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.

[Feature Request]: Add title to the PageHeader subtitle
5 participants