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

add better text truncation method #8034

Merged

Conversation

michaelchadwick
Copy link
Contributor

References ilios/ilios#3482

My first crack at changing our current <TruncateText> system to a <FadeText> system: instead of removing HTML and squishing text together, leave it all as-is but put a fade-to-bg effect on it. Only working on Session Objectives->Description for now, but could be expanded.

Old, Tired, Non-Ideal:
Screenshot 2024-08-01 at 4 24 32 PM
New, Wired, Superior(?):
Screenshot 2024-08-01 at 4 28 07 PM

The vertical spacing it takes up can be adjusted. Here's another one with more faded away:
Screenshot 2024-08-01 at 4 31 20 PM

@jrjohnson
Copy link
Member

I like it on sessions, needs some attention on course objectives (and probably generically shouldn't specify it's own background, but should fade into whatever is behind it if possible)
Screenshot 2024-08-06 at 3 16 19 PM

@michaelchadwick michaelchadwick changed the title Frontend 3482 better text truncation add better text truncation method Aug 6, 2024
@michaelchadwick
Copy link
Contributor Author

@jrjohnson Good catch. I'm also realizing <TruncateText> is used in a few more places that I need to account for, so back to the drawing board.

@michaelchadwick
Copy link
Contributor Author

Places where <TruncateText> is currently used and <FadeText> could replace:

  • CourseObjectiveList
    • ObjectiveSortManager
  • SessionObjectiveList
    • OfferingManager (this seems like it shouldn't change; only location, so usually short)
  • SessionsGridOffering (this seems like it shouldn't change: only location, so usually short)
  • WeekAtAGlance, as seen on an event's description (this seems like it shouldn't change: not in a grid)
    • LearningMaterialListItem, as seen on WaaG in an LM's "Public Notes" (this could use some discussion, as it's essentially the same as the parent list item, except that it displays slightly differently when expanded)
  • MaterialListItem (this seems it shouldn't change: instructor name(s))

@michaelchadwick
Copy link
Contributor Author

@jrjohnson I believe I fixed the background color issue, but had to be specific instead of general (thankfully, there are only three places to be specific about right now).

@michaelchadwick michaelchadwick marked this pull request as ready for review August 12, 2024 14:52
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

Needs team discussion (and a couple tweaks).

@michaelchadwick
Copy link
Contributor Author

Note to self: attempt to create a mixin that can be used to facilitate the gradient from transparent->text background color.

@michaelchadwick
Copy link
Contributor Author

@jrjohnson I moved the component-specific styles in fade-text.scss into their respective component style files, which makes more sense. Two of them had to be in addition to the mixin they included because of different background colors, whereas the other one I was able to add to the mixin itself.

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

I really love how this looks, I found a couple of new behavior / visual issues in testing:

  1. Clicking on the unexpanded text does nothing. In the current implementation if you click on a collapsed objective the editor opens. I think that is good, but I'd also accept that clicking anywhere in the text expanded the entire block.
  2. If the text is indented (like in a list) the control floats up into the middle instead of being at the bottom, this isn't an issue with our current control because the indentation never happens due to the HTML stripping:
Screenshot 2024-08-21 at 12 30 53 PM 3. Because we're using text length to collapse, but the actual behavior is height based it's a little confusing why only some things collapse (this may only be in tests and not in real life, and may not be fixable): Screenshot 2024-08-21 at 12 37 17 PM

Not sure if we should go ahead with these or try and find fixes. Let's discuss.

@michaelchadwick
Copy link
Contributor Author

@jrjohnson All good points. Will look into these.

@michaelchadwick michaelchadwick force-pushed the frontend-3482-better-text-truncation branch from 3f09ad8 to 549155f Compare August 23, 2024 18:40
@michaelchadwick
Copy link
Contributor Author

@jrjohnson I fixed all the stuff mentioned in point 1, but not sure how to approach point 2 without re-architecting the whole truncation system.

@michaelchadwick
Copy link
Contributor Author

@jrjohnson This has one failing testapp test, because it's testing for truncation length, and not the new visual fading thing. Not sure how to rewrite test, hmmm.

https://github.com/ilios/frontend/blob/master/packages/test-app/tests/integration/components/editable-field-test.js#L143-L154

@michaelchadwick michaelchadwick force-pushed the frontend-3482-better-text-truncation branch from 549155f to 850a9a2 Compare August 26, 2024 16:38
@michaelchadwick michaelchadwick force-pushed the frontend-3482-better-text-truncation branch from 850a9a2 to 241effd Compare September 3, 2024 22:08
@michaelchadwick
Copy link
Contributor Author

Having some issues with getting a JS triggered click to target the correct item (thus, test won't pass). Manually clicking with a mouse/touch works fine, though. Frustrating -_-

@michaelchadwick michaelchadwick force-pushed the frontend-3482-better-text-truncation branch from f0b3e9d to ee055e5 Compare September 25, 2024 16:12
@michaelchadwick
Copy link
Contributor Author

@dartajax I finally got every test to pass; just have Percy to finish. Can you take a look, and bring this one home? Thanks a bunch.

Copy link
Member

@dartajax dartajax left a comment

Choose a reason for hiding this comment

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

looks good to me

@dartajax dartajax merged commit 54e8379 into ilios:master Sep 26, 2024
51 checks passed
@michaelchadwick michaelchadwick deleted the frontend-3482-better-text-truncation branch September 26, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run ui tests Run the expensive UI tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session Objectives Having Bulleted Or Numbered Lists Need Better Formatting - Courses and Sessions
3 participants