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

feat(quantic): make quantic notifications component dismissible #4733

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

SimonMilord
Copy link
Contributor

@SimonMilord SimonMilord commented Nov 27, 2024

SFINT-5767

IN THIS PR:

  • Fixed a styling issue we had with the notifications where the width would look weird.
  • Made the quantic notifications component dismissible, thus following best practices by Salesforce and offering a better user experience.
  • Made sure: that if you dismiss notifications and re-trigger the same query, that they would re-appear
  • Made sure: that if you dismiss notifications and trigger a new search, the previous notifications would not "flash" before updating the state

More about the solution:

The QuanticNotifications component was rendering the notifications based on the notifications array it was getting from the notify trigger state from headless. We are therefore adding a button to close the notification in the template, which will call the handleNotificationClose handler function.

DEMO STYLING:

Before:
Screenshot 2024-11-27 at 4 59 35 PM

After:
image

DEMO FUNCTIONALITY:

Screen.Recording.2024-11-29.at.2.46.13.PM.mov

BONUS:

Resized the default icon for x close button to 16px instead of 24px which looks nicer:

image

TESTS:

image

Copy link

github-actions bot commented Nov 27, 2024

Pull Request Report

PR Title

❌ Title should follow the conventional commit spec:
<type>(optional scope): <description>

Example: feat(headless): add result-list controller

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 242.8 242.8 0
commerce 347.7 347.7 0
search 413.9 413.9 0
insight 405 405 0
recommendation 255.1 255.1 0
ssr 407.4 407.4 0
ssr-commerce 362.7 362.7 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

@mmitiche
Copy link
Contributor

Visual review:

Nice work 👍
I think the dismiss button a bit huge, it will look much better in my opinion if we make it smaller, like the size of the button to clear the query in the search box for example.

@SimonMilord
Copy link
Contributor Author

Visual review:

Nice work 👍 I think the dismiss button a bit huge, it will look much better in my opinion if we make it smaller, like the size of the button to clear the query in the search box for example.

Set the height of the icon to 16px which looks nicer! thanks

Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

Some small improvements that I think can help clarity

Comment on lines 85 to 94
if (!this.searchStatus.state.isLoading) {
this.notifications =
this.notifyTrigger?.state?.notifications.map((notification, index) => ({
value: notification,
id: index.toString(),
visible: true,
})) ?? [];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would do that, that way on each "new query" the notifications are actually cleared so for example they don't display while the rest of the UI is in "loading" mode, aka showing placeholders.

Suggested change
if (!this.searchStatus.state.isLoading) {
this.notifications =
this.notifyTrigger?.state?.notifications.map((notification, index) => ({
value: notification,
id: index.toString(),
visible: true,
})) ?? [];
}
if (this.searchStatus?.state?.isLoading) {
this.notifications = [];
} else {
this.notifications =
this.notifyTrigger?.state?.notifications.map((notification, index) => ({
value: notification,
id: index.toString(),
visible: true,
})) ?? [];
}

Also why did you add the toString() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will test the first point you bring up, seems to make sense

for the second question, it is because in the handleNotificationClose when we do: if (notification.id === currentNotificationId), the currentNotificationId is of type string, so it was so that they could do the comparison, but when I think about it, I could probably just do if (notification.id.toString() === currentNotificationId)

@@ -1,7 +1,15 @@
:host {
--slds-c-toast-sizing-min-width: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I documented that in the PR description, it is to fix the width issue of the notifications

}

.notification-container {
z-index: 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally would put this at like 10?
So at least there can be content in between 0 and the notifications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modal is 40 so we could do steps of 20 and set it at 20, but I didnt think there would be content between 0 and 1

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.

3 participants