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

Properly render 'expand' and 'collapse' action on notification #10471

Merged
merged 5 commits into from
Dec 1, 2021
Merged

Properly render 'expand' and 'collapse' action on notification #10471

merged 5 commits into from
Dec 1, 2021

Conversation

martin-fleck-at
Copy link
Contributor

What it does

  • Fix assignment of class names
  • Scale icons to better fit the 'close icon'
    -- Remove hover background as due to the scaling it looks weird
    -- Use scaling as indicator instead

fix_expandCollapse

Fixes #10470

How to test

  1. Produce a message over 500 characters (for example through a sample menu contribution)
  2. See that the expand/collapse action is there and working

Review checklist

Reminder for reviewers

- Fix assignment of class names
- Scale icons to better fit the 'close icon'
-- Remove hover background as due to the scaling it looks weird
-- Use scaling as indicator instead
@vince-fugnitto vince-fugnitto added the notifications issues related to notifications label Nov 26, 2021
@martin-fleck-at
Copy link
Contributor Author

martin-fleck-at commented Nov 26, 2021

@vince-fugnitto Thank you for your fast review! I removed the adaptation again and just reduced the scaling a bit to make the icon size match the close icon a bit better. If you want I can remove that as well though. I double checked and I actually made the collapse/expand an action item which it was not before. Without that we do not have a background on which the scaling looked weird.

fix_expandCollapse2

@vince-fugnitto
Copy link
Member

@martin-fleck-at I haven't tested your latest changes yet, but based on the gif you sent something still looks incorrect. The behavior should be like the close icon, and align with vscode:

expand-icon.mp4

@martin-fleck-at
Copy link
Contributor Author

@vince-fugnitto Got it! I updated the change but I removed the scaling now because otherwise it looks weird:

fix_noScale
(without scaling and submitted)

fix_scale
(with the scaling defined in master)

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Great, thanks for taking care of that.

Something didn't seem quite right about the direction of the chevron both in expanded and collapsed form. I took a look at how VSCode handles this. Seems like my intuition proved correct :)

Collapsed:

image

Expanded:

image

Do you mind adjusting the rotation of the chevron to align with VSCode? Afterwards, this looks good to go.

@martin-fleck-at
Copy link
Contributor Author

@msujew Very good catch! Thank you! I aligned the chevron direction accordingly:

arrow

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

  • The icon is visible again after your changes. (compared to master)
  • The chevron does not increase its size on hover/click.
  • The chevron rotation is aligned with vscode for expansion/collapse/hover.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
The only difference I noticed was that the icons were only visible on hover in vscode, but we do not need to handle that in this pull-request.

@vince-fugnitto vince-fugnitto merged commit 19008d9 into eclipse-theia:master Dec 1, 2021
@vince-fugnitto vince-fugnitto added this to the 1.21.0 milestone Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notifications issues related to notifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification expand/collapse is no longer shown
3 participants