feat: apply ellipsis into notification #2199
Conversation
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 3.14% (+0% 🔼) |
101/3219 |
| 🔴 | Branches | 3.32% (+0% 🔼) |
67/2018 |
| 🔴 | Functions | 1.55% (+0% 🔼) |
17/1097 |
| 🔴 | Lines | 3.2% (+0% 🔼) |
101/3160 |
Test suite run success
20 tests passing in 4 suites.
Report generated by 🧪jest coverage report action from e16331b
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| const { t } = useTranslation(); | ||
| const { token } = theme.useToken(); | ||
| const [showExtraDescription, setShowExtraDescription] = useState(false); | ||
| const [ellipsis, setEllipsis] = useControllableValue({ | ||
| defaultValue: allowEllipsis, | ||
| }); |
There was a problem hiding this comment.
useControllableValue helps manage the state to be either self-managed or controlled by its parent.
allowEllipsis is not applicable in this case. allowEllipsis determines whether ellipsis is allowed or not. Ellipsis refers to the current state of being ellipsised or not, which is not related to the allowance of ellipsis.
There was a problem hiding this comment.
Also, you might not need to manage the ellipsis status and don't need to handle it using the click event. Typography has a feature to manage it, such as the expandable property. Please check this.
There was a problem hiding this comment.
The allowEllipsi is simply a property that turns ellipsis on/off, so I didn't think it was right to use a useControllableValue, so I removed it.
The expendable property only provides the ability to expand the omitted text again, not to omit the expanded text again. So in the sider that holds all the notifications, I want to support both collapsing and expanding, so I'm controlling the ellipsis property with a click event.
There was a problem hiding this comment.
It's okay if it can't use ellipsis again. The main points are:
- The clickable content area is good for the main action. For example, clicking the "success creating session" notification takes you to the session detail page. It's better not to use the clicking action for toggling. Let's reserve clicking for another feature later.
- The UX for toggling ellipsis by clicking again is uncommon and not explicit.
There was a problem hiding this comment.
Could you let me know what the longest notification you have found is?
There was a problem hiding this comment.
@yomybaby
I decided that scalability would be an afterthought, so I modified the code to focus on fixing the mentioned issues.
ref The error occurs when turn off storage and access the web. I didn't find the error in my test environment, so I tested it by hardcoding the notification text for the usage calculation part.
This PR resolves #2196
feature :
Checklist: (if applicable)