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

frontend: Fix notifications format styles #1674

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Conversation

vyncent-t
Copy link
Contributor

@vyncent-t vyncent-t commented Jan 25, 2024

Enhancements to Notifications List View for Better UX

Solves issue #1667

Description

This PR focuses on improving the user experience of the Notifications List View. It involves utilizing the available horizontal space more effectively to display a larger portion of the notification message. Additionally, the cluster name in each notification will now be a clickable link, redirecting users directly to the associated cluster.

Changes

  • Expanded the message display area in the Notifications List View to make better use of horizontal space.
  • Converted cluster names in notifications into hyperlinks that lead to the respective clusters.

Verification

  • Ensured that more of the notification message is visible in the expanded view.
  • Tested the functionality of the new cluster links to confirm they correctly redirect to the appropriate cluster.

Screenshots

image

@vyncent-t vyncent-t self-assigned this Jan 25, 2024
@vyncent-t vyncent-t force-pushed the fix-notifications-lay branch from 2c0e2cb to 005f66e Compare January 25, 2024 20:43
@vyncent-t
Copy link
Contributor Author

Dev notes

  • This push is a solid base, I have tweaked the vw for each size to not clip or run into other columns (ignore the colors)

  • I am using the CellProps and a custom style map to put in custom style changes to achieve this
    image
    image

  • Currently using useEffect to rerender when any screen size changes, when it changes it grabs a new view size for the message column
    image

  • The comps are responsive
    Large
    image

Medium
image

Small
image

@vyncent-t vyncent-t force-pushed the fix-notifications-lay branch 2 times, most recently from 4a06533 to 3bb8730 Compare January 30, 2024 19:45
@vyncent-t
Copy link
Contributor Author

Dev note

  • Added context links to the notifications view

image

@vyncent-t vyncent-t requested a review from illume January 30, 2024 20:09
@vyncent-t vyncent-t force-pushed the fix-notifications-lay branch from 3bb8730 to d4bffe8 Compare February 5, 2024 17:35
@vyncent-t
Copy link
Contributor Author

vyncent-t commented Feb 5, 2024

Dev note

  • created more space for notification message and context link
  • shrank the space needed for the color signals
  • removed box style for context link

image

@vyncent-t vyncent-t added the frontend Issues related to the frontend label Feb 5, 2024
@vyncent-t vyncent-t force-pushed the fix-notifications-lay branch from d4bffe8 to 29606e6 Compare February 8, 2024 14:45
@vyncent-t vyncent-t marked this pull request as ready for review February 8, 2024 14:45
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

The PR approach has an issue with cutting off parts of the table. See the screenshot below:

Screenshot showing the prob

I think we can accomplish a good result with a less engineered effort here. Namely by using gridTemplate: for each column as adequate. The text column should just have auto so it expands, and the rest should have a min-content.
The div around the text column should not have the width={'30vw'}, otherwise it looks artificially cut.

@vyncent-t vyncent-t force-pushed the fix-notifications-lay branch from 29606e6 to 66e3570 Compare February 16, 2024 21:06
@vyncent-t vyncent-t changed the title frontend Notifications/List.tsx: Fix and reformat styles frontend: Fix notifications format styles Feb 16, 2024
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

LGTM. The mobile version is not functional but we should fix that in a different occasion anyway.

@joaquimrocha joaquimrocha merged commit 7e84e71 into main Feb 26, 2024
9 checks passed
@joaquimrocha joaquimrocha deleted the fix-notifications-lay branch February 26, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues related to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants