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

Update Notification API to use modern looking X #2037

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Jul 5, 2024

The find/replace overlay introduces a new close icon similar to the close icon used for the tab folder from
https://bugs.eclipse.org/bugs/show_bug.cgi?id=575395

This pull request updates the notification close icon which still looks like the old tab icon before the change in
https://bugs.eclipse.org/bugs/show_bug.cgi?id=575395 and does not fit anymore in the overall look and feel.

A follow-up PR could also update the active folder but find a new "reddisch" color would require more testing on the different OS, while with the regular icon we can build on the testing of #1991

Fixes: #2035

Copy link
Contributor

github-actions bot commented Jul 5, 2024

Test Results

 1 815 files  + 2   1 815 suites  +2   1h 31m 8s ⏱️ - 5m 31s
 7 663 tests ± 0   7 435 ✅ + 2  228 💤 ±0  0 ❌  - 2 
24 150 runs  +18  23 401 ✅ +20  749 💤 ±0  0 ❌  - 2 

Results for commit e8de8bb. ± Comparison against base commit 80c6da9.

♻️ This comment has been updated with latest results.

@BeckerWdf
Copy link
Contributor

can you please post before and after screenshots?

@vogella
Copy link
Contributor Author

vogella commented Jul 5, 2024

can you please post before and after screenshots?

You see the icons here: https://github.com/eclipse-platform/eclipse.platform.ui/pull/2037/files. It this sufficient?

vogella added 2 commits July 5, 2024 13:34
The find/replace overlay introduces a new close icon similar to the
close icon used for the tab folder from
https://bugs.eclipse.org/bugs/show_bug.cgi?id=575395

This pull request updates the notification close icon which still looks
like the old tab icon before the change in
https://bugs.eclipse.org/bugs/show_bug.cgi?id=575395  and does not fit
anymore in the overall look and feel.

A follow-up PR could also update the active folder but find a new
"reddisch" color would require more testing on the different OS, while
with the regular icon we can build on the testing of
eclipse-platform#1991

Fixes: eclipse-platform#2035
@vogella vogella force-pushed the notification-close-icon-regular-case branch from 6b52ee1 to e8de8bb Compare July 5, 2024 11:34
@BeckerWdf
Copy link
Contributor

I meant screenshot of the notification before and after.
Best in light and dark theme.
Just like @HeikoKlare always does it.

@Wittmaxi
Copy link

Wittmaxi commented Jul 5, 2024

Should we also update the .images repository?

@BeckerWdf
Copy link
Contributor

Should we also update the .images repository?

Yes please.

@vogella
Copy link
Contributor Author

vogella commented Jul 5, 2024

New Light:

image

New Dark:

image

Old Light:

image

Old Dark:

image

As you notice the notification ignores the styling, independent of the icon. I never noticed that before that that is an unrelated issue.

@HeikoKlare
Copy link
Contributor

I just tested the PR and with my setup (running the NotificationPopupTest on Windows) the popup actually adapts to the theme. Here is what is looks like:
Screenshot 2024-07-05 154955
image

In any case, the icon looks fine for me, even though it could be a bit lighter.

@vogella vogella merged commit 44dc49e into eclipse-platform:master Jul 5, 2024
9 of 12 checks passed
@vogella
Copy link
Contributor Author

vogella commented Jul 5, 2024

In any case, the icon looks fine for me

thanks :-)

@vogella vogella deleted the notification-close-icon-regular-case branch July 5, 2024 18:44
@vogella
Copy link
Contributor Author

vogella commented Jul 5, 2024

Should we also update the .images repository?

Yes please.

@HeikoKlare once you add your svg to the image repo, can you also also add a copy to the notification project?

@HeikoKlare
Copy link
Contributor

@HeikoKlare once you add your svg to the image repo, can you also also add a copy to the notification project?

I have already added the close icon for the find/replay overlay to the images repo (eclipse-platform/eclipse.platform.images#80), but I can, of course, create another PR for also adding the icon to the jface.notifications project.

When trying to do so, I found that there is also an "active" version of the icon, which is used when hovering over it. We need to adapt this too, as currently the old icon is still used when hovering (and I do not think that we can easily get rid of it, as we hovering feedback has to be implemented in some way):
notification_close_active

I will have a look and try to provide a proper new "active" icon.

@Wittmaxi
Copy link

Wittmaxi commented Jul 6, 2024

@HeikoKlare related to my report in #2043

@HeikoKlare
Copy link
Contributor

@HeikoKlare related to my report in #2043

Thanks! I missed that one.

HeikoKlare added a commit to HeikoKlare/eclipse.platform.images that referenced this pull request Jul 6, 2024
Updates the icons used by JFace notifications. Also removes the (unused
and equal) disabled version of the icons.

See eclipse-platform/eclipse.platform.ui#2037
See eclipse-platform/eclipse.platform.ui#2048
HeikoKlare added a commit to eclipse-platform/eclipse.platform.images that referenced this pull request Jul 7, 2024
Updates the icons used by JFace notifications. Also removes the (unused
and equal) disabled version of the icons.

See eclipse-platform/eclipse.platform.ui#2037
See eclipse-platform/eclipse.platform.ui#2048
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.

find/replace overlay - Notification API uses old X
4 participants