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

fix: improve styling of messages #390

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

Conversation

danditomaso
Copy link
Collaborator

  • Modified message layout to improve alignment of Avatar and longName
  • Maintained existing message styling and state indicators (ack, waiting, error)
  • Improved alignment of message text to long name
Screenshot 2025-01-29 at 4 14 12 pm Screenshot 2025-01-29 at 4 14 18 pm Screenshot 2025-01-29 at 4 14 23 pm

@dzienisz
Copy link
Contributor

Can you add before/after screenshots? How does it look on mobile screens?

@danditomaso danditomaso changed the title fix: improve styling of messsges fix: improve styling of messages Jan 30, 2025
@danditomaso
Copy link
Collaborator Author

@Hunter275 Let me know if there are any concerns with this PR, we should get it merged before issuing a new release to fix some avatar alignment issues.

@Hunter275 Hunter275 added the minor change This is a minor change label Jan 31, 2025
@Hunter275
Copy link
Member

The background on the selected page is being shortened

image

@KomelT
Copy link
Contributor

KomelT commented Feb 3, 2025

Nice touch would be to show tool tip when hovering over the status. "waiting for acknowledgment", "delivered"... Like on mobile.
image

@danditomaso
Copy link
Collaborator Author

Nice touch would be to show tool tip when hovering over the status. "waiting for acknowledgment", "delivered"... Like on mobile. image

I like this idea, but you cant use a hover state on mobile browser. :(

@danditomaso
Copy link
Collaborator Author

danditomaso commented Feb 3, 2025

@dzienisz What do you think of this? When you hover your mouse over the delivery icon you'll get the status of the message.
image

image

@danditomaso danditomaso marked this pull request as draft February 3, 2025 18:24
@danditomaso danditomaso marked this pull request as ready for review February 3, 2025 21:56
@Hunter275
Copy link
Member

If we removed the Traceroute feature, is the user able to do that in another place now?

@Hunter275
Copy link
Member

Can we decrease the spacing between the Node name and the message?

Also I really like the color transition from the ACK but I'm not a fan of the | pipe on each message. That style is mainly used for quotes.

Like this

It might work to use the pipe as the visual component for the ACK. It starts yellow and transitions to green after the ACK.

image

@dzienisz
Copy link
Contributor

dzienisz commented Feb 4, 2025

@dzienisz What do you think of this? When you hover your mouse over the delivery icon you'll get the status of the message. image

image

I like this! With these kind of features, we always need to watch what users think about it and iterate over it.

Otherwise, it's good to have some UX/UI person onboard.

I feel that this iteration is a good move forward, but I would spend some time in the future to work more on this. It's a critical place in application as this is the place where people communicate with each other.

@dzienisz
Copy link
Contributor

dzienisz commented Feb 4, 2025

If we removed the Traceroute feature, is the user able to do that in another place now?

Great question. I don't know yet enough about it. Maybe @danditomaso could help us?

@KomelT
Copy link
Contributor

KomelT commented Feb 4, 2025

@Hunter275 @dzienisz, traceroute will be added into nodes pages. With #307 PR.

@danditomaso
Copy link
Collaborator Author

The background on the selected page is being shortened

image

That has been fixed in this PR, not sure when this regression was introduced. We absolutely need ui testing to ensure these types of changes are preventing from being introduced going forwards.

@danditomaso
Copy link
Collaborator Author

danditomaso commented Feb 4, 2025

@KomelT @dzienisz Traceroute was removed from the DM Message
page because the UI for it was awful. You couldn't close the panel once it was opened, it took up over half the screen and there wasn't any explanation what you were clicking. It really was the wrong place for it.

Going forwards I can absolutely see having these tools grouped in Messages inside some sort of "Actions" dropdown, to avoid cluttering the toolbar. Also I would love to have a UI/UX designer available, but since we're all volunteers its up to use to use our respective skillsets to contribute code, I come from a UI/UX background as I have been a frontend dev for 5+ years now, but wouldn't call myself a designer

@danditomaso
Copy link
Collaborator Author

Can we decrease the spacing between the Node name and the message?

Also I really like the color transition from the ACK but I'm not a fan of the | pipe on each message. That style is mainly used for quotes.

Like this

It might work to use the pipe as the visual component for the ACK. It starts yellow and transitions to green after the ACK.

image

@Hunter275 I can make those changes this morning. So i'm decreasing the space each message/message group takes up, and removing the pipe, or having it change change color based on the status of the message

@dzienisz
Copy link
Contributor

dzienisz commented Feb 4, 2025

I think some UX/UI designers could volunteer, but we should actively look for them. Software is not just backend but also frontend 😅

@danditomaso
Copy link
Collaborator Author

I think some UX/UI designers could volunteer, but we should actively look for them. Software is not just backend but also frontend 😅

If you know anyone please have them help out, but in the mean time I come from a frontend dev background, which is why I have been contributing so many PR's to this specific repo.

@Hunter275
Copy link
Member

@danditomaso I think leaving it with the font color is best. That's what other apps are doing.

Is there a font color for items that failed to ACK?

@danditomaso
Copy link
Collaborator Author

@danditomaso I think leaving it with the font color is best. That's what other apps are doing.

Is there a font color for items that failed to ACK?

There currently isn't but I can add one. Do you like red or would you prefer like a shade of grey be used?

@danditomaso
Copy link
Collaborator Author

@Hunter275 Alright, I've made the changes you requested please see the screenshots.

The error state
Screenshot 2025-02-04 at 2 33 29 pm

A conversation
Screenshot 2025-02-04 at 2 32 05 pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change This is a minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants