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

PCH Smart Linking: Update UI #2268

Merged
merged 3 commits into from
Mar 8, 2024
Merged

PCH Smart Linking: Update UI #2268

merged 3 commits into from
Mar 8, 2024

Conversation

vaurdan
Copy link
Contributor

@vaurdan vaurdan commented Mar 7, 2024

Description

Update the UI in the Smart Linking panels of the Editor Sidebar.

There's a minor deviation from the proposed design, as the Snackbars do not have icons. Unfortunately, I wasn't able to place them in the correct position, and without the ability to edit their CSS, they it's basically unfixable.

Motivation and context

How has this been tested?

Tested locally with all the new changes validated.

@vaurdan vaurdan added this to the 3.14.0 milestone Mar 7, 2024
@vaurdan vaurdan self-assigned this Mar 7, 2024
@vaurdan vaurdan requested a review from a team as a code owner March 7, 2024 17:59
Copy link
Collaborator

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Excellent work! Let's merge this as-is!

@acicovic
Copy link
Collaborator

acicovic commented Mar 8, 2024

@dabowman, some notes for our patch iteration:

  1. The hint that's in the design doesn't seem to be implemented here. Having said that, I doubt we need it due to how the ToggleGroupControl works. Check it out and let us know if you think it's still good to add it.
  2. We're now showing a SnackBar, but we're also still showing a Notice. I don't think it's necessarily an issue. The SnackBar dissapears and the Notice is a way to see what was done after that. Also the SnackBar is less visible and can be missed in my personal experience. Let us know if you think the Notice should still be removed though.

@vaurdan
Copy link
Contributor Author

vaurdan commented Mar 8, 2024

We're now showing a SnackBar, but we're also still showing a Notice. I don't think it's necessarily an issue. The SnackBar dissapears and the Notice is a way to see what was done after that. Also the SnackBar is less visible and can be missed in my personal experience. Let us know if you think the Notice should still be removed though.

This was exactly why I decided to keep the notice despite of the Snackbar. While testing, I found myself missing the Snackbar multiple times despite being looking out for it.

For errors, I think the Notice is better suited to display the error message than the Snackbar.

@vaurdan vaurdan merged commit 7457e25 into redesign Mar 8, 2024
34 checks passed
@vaurdan vaurdan deleted the update/smart-linking-ui branch March 8, 2024 09:47
@acicovic acicovic mentioned this pull request Mar 11, 2024
@dabowman
Copy link

Yeah, the snackbar is a but subtle. Is there a way to make the notice dismissible? or make it disappear after a few seconds? That might be nice if we're going to include the notice in the sidebar. I wish there was a less visual way to give a confirmation. The notice component is just a bit much, visually. But it does it's job.

I'm torn on the hint for smart linking. I don't love not being able to click the selected block option without recieving any feedback. I think there are a couple ways we could do it and we might need to try it a couple ways and test a bit. I think we can let the user select "selected blocks" but then give a hint when they try to hit submit if a block isn't selected. That's how I designed it in Figma.

I think the other option is to place the hint under the toggle group when the user tries to click "selected blocks" without any blocks selected. So the behavior is the same as it is currently with the addition of a hint if they don't have a selected block. After using things a bit I think this is what i like more.

@acicovic
Copy link
Collaborator

acicovic commented Mar 12, 2024

Is there a way to make the notice dismissible?

Should be. The Notice component has an isDismissible property.

@vaurdan
Copy link
Contributor Author

vaurdan commented Mar 26, 2024

After #2313 and #2324, the Notice has been made dismissible, and the hint is now shown when trying to select "Selected Block" without any current selected block in the editor.

@acicovic
Copy link
Collaborator

I consider the patch iteration comments in this thread as addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants