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

Notifications page & dropdown improvements #61

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

Notifications page & dropdown improvements #61

wants to merge 112 commits into from

Conversation

xbpcb
Copy link
Collaborator

@xbpcb xbpcb commented Feb 4, 2025

Fix #43

@xbpcb xbpcb requested a review from alexey-yarmosh February 20, 2025 08:30
.n-accordion-panel_new .n-header-subj {
@apply !text-[var(--bluegray-900)];
@apply dark:!text-[var(--bluegray-0)];
Copy link
Member

Choose a reason for hiding this comment

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

Base fixes of text colors and backgrounds, can be applied directly at preset files. So for something specific => local overrides, for changes like replacing text-surface-800 with text-bluegray-900 => global theme override.

Copy link
Member

Choose a reason for hiding this comment

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

@MartinKolarik since there is #61 (comment) could you please confirm that is ok? In 90% of the cases it is only fixing colors for dark theme (e.g. https://github.com/jsdelivr/globalping-dash/pull/59/files#diff-6fd410bbd76536ad0916987fb9292b0fcc746a5b7becc71554e20988d30979bc)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, fixing theme colors most likely applies to all occurrences of the component so preset.

@MartinKolarik
Copy link
Member

MartinKolarik commented Feb 24, 2025

  • please update the unread label style as per Figma, and show it (and the "mark all as read") only if there are some unread notifications.
  • let's add the unread count to the page as well, left of the button

Copy link
Member

@MartinKolarik MartinKolarik left a comment

Choose a reason for hiding this comment

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

Almost perfect now, just a bit more CSS cleanup.

Comment on lines -201 to +339
.notification p {
.notification-content-msg p {
margin-bottom: 18px;
word-break: break-all;
}

.notification p:last-child {
.notification-content-msg p strong {
word-break: break-all;
}

.notification-content-msg p:last-child {
margin-bottom: 0;
}

.notification a {
.notification-content-msg a {
color: var(--p-primary-color);
font-weight: 600;
}

.n-popover[data-pc-name="popover"] {
@apply !rounded-xl;
@apply !overflow-hidden;
@apply absolute !ml-4 !mt-2;
}
Copy link
Member

Choose a reason for hiding this comment

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

This part should be done in tailwind as well.

Comment on lines +341 to +345
.n-popover[data-pc-name="popover"] > *[data-pc-section="content"] {
@apply flex items-center;
@apply !rounded-xl;
@apply border dark:border-[var(--table-border)];
}
Copy link
Member

Choose a reason for hiding this comment

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

Here it can be done too via :pt={ content: { class: '...' } } on the popover component

Comment on lines +406 to +423

.notification-new-icon {
width: 8px;
height: 8px;
border-radius: 50%;
}

.n-accordion-panel[data-pc-name="accordionpanel"] {
@apply !rounded-xl;
}

.n-expand-chevron {
transition: all 400ms ease-out;
}

.n-accordion-panel [aria-expanded="true"] .n-expand-chevron {
transform: rotate(90deg);
}
Copy link
Member

Choose a reason for hiding this comment

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

All tailwind.

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.

Notifications page & dropdown improvements
3 participants