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

Replace submitted note table colors with created/resolved #5269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

Instead of submitted and commented note colors switch to created, resolved and commented.

Reasons:

  • Close actions were colored as commented even if notes were closed without any comments. Now they are going to be colored as resolved.
  • Submitted is not a term we use for notes, we have "add a note" and notes "created by".
  • There was a complaint about the primary color used for submitted notes that is blue making links harder to see. What other color can we use? For example, the success color, like we do for unread messages. But that's the color of the resolved note marker, and then we might as well add the unresolved color.

Caveats:

  • I keep "submitted" in locale keys to avoid errors while subheading_html is not updated.
  • I ignore reopen events because if I don't, I'd have to treat them as open events but then there wouldn't be an equivalent to the previous submitted state. And the equivalent is green created + yellow created and resolved.
  • I had to add this created and resolved state because what if the user did both actions? I can't pick one more important color. If I look at note status changes, the latest action is the most important one and I'd have to color notes as resolved. But then again I'd lose the equivalent to the previous submitted state.

image

Dark mode:
image

Legend while locale strings aren't updated:
image

@tomhughes
Copy link
Member

You say you ignore reopen events but in fact this seems to extend the concept of submitted/created from who initially opened it to include anybody that reopens it as well? You are considered to have created it if there are any open events by you.

Obviously that is mirrored in terms of closing by looking for any close event, but aren't users going to be more interested in whether the note is currently open or closed and whether this person closed it?

@AntonKhorev
Copy link
Collaborator Author

You say you ignore reopen events but in fact this seems to extend the concept of submitted/created from who initially opened it to include anybody that reopens it as well?

Are you proposing to treat reopen events exactly like initial open events? That's not what we're currently doing with the blue submitted coloring. It only looks at the initial open event, or actually it does the equivalent thing of checking the author (the user who did the initial opening).

@tomhughes
Copy link
Member

tomhughes commented Oct 18, 2024

After your change the test is:

opened_by_user = note.comments.any? { |comment| comment.author == @user && comment.event == "opened" }

which will be true if the user has ever opened it, either the original open or a reopen?

@AntonKhorev
Copy link
Collaborator Author

This is a test only for original open events:

comment.event == "opened"

Reopen events are stored as "reopened".

@AntonKhorev
Copy link
Collaborator Author

AntonKhorev commented Oct 18, 2024

Another option is to ignore all close events unless it's the last event on the note, that's symmetrical to ignoring reopen events.

@tomhughes
Copy link
Member

Oh sorry I looked at the source last night and was quite sure they were all marked as opened.

@tomhughes
Copy link
Member

Why check every comment though when the initial open will always be the first one?

There's still a question about resolution I think? Should we be looking at every one or just the last one?

@AntonKhorev
Copy link
Collaborator Author

There are some drawbacks in looking only at the last close event.

#5255 tried to do filtering by interaction type but the db queries are ugly. To optimize them we could have a users x notes table where we store among other things interaction flags. When a user closed a note, we set a closed flag there, then we can filter notes by that flag. But if we want to pay attention only to the last close event, we'd have to clear that flag when somebody reopens the note.

@tomhughes
Copy link
Member

My thinking was literally just to look at the last event which should be the close event unless it's been reopened and if it has then do we want to consider it closed by this user?

@AntonKhorev
Copy link
Collaborator Author

Alright, let's not think about filtering for now. Changed to look only at the last close event.

I also wanted the "commented" indication not to be misleading. It includes more than commenting and this change stretches it further by including non-last close events. So I removed the claim that not colored notes were commented, although the sentence gets a bit long:

image

@AntonKhorev AntonKhorev added the notes Related to the notes feature label Oct 25, 2024
@AntonKhorev AntonKhorev mentioned this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes Related to the notes feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants