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.

Sign up for GitHub

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

Jump to bottom

ui-spacetimechart: add conflict tooltips #660

Merged
merged 5 commits into from
Oct 31, 2024
Merged

ui-spacetimechart: add conflict tooltips #660

merged 5 commits into from
Oct 31, 2024

Conversation

emersion
Copy link
Member

@emersion emersion commented Oct 22, 2024 •
edited
Loading

@emersion emersion force-pushed the emr/conflict-tooltips branch 3 times, most recently from cc113fd to 4ef6cf8 Compare October 23, 2024 09:57
@emersion emersion requested review from Synar and clarani October 23, 2024 14:45
@emersion
Copy link
Member Author

This is feature complete, but leaving as a draft PR since I have one UX question: I don't think it makes sense to have multiple conflicts listed in the tooltip. Will ask around. Shouldn't have a lot of impact either way.

Synar
Synar approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@Synar Synar left a comment

Choose a reason for hiding this comment

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

Nice, LGTM

I will try do another pass tomorrow to find something to nitpick about though ^^

clarani
clarani approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@emersion emersion force-pushed the emr/conflict-tooltips branch from 4ef6cf8 to 40e54ab Compare October 29, 2024 15:48
@emersion
Copy link
Member Author

emersion commented Oct 29, 2024 •
edited
Loading

Updated to follow the latest mockup. Also clarified that the time displayed in green text is wired up to the cursor, it's not the conflict start time. (Reviewers: only the last commit has been edited.)

@emersion emersion marked this pull request as ready for review October 29, 2024 15:49
@emersion emersion requested a review from a team as a code owner October 29, 2024 15:49
@emersion emersion requested review from clarani and Synar October 29, 2024 15:49
@emersion emersion force-pushed the emr/conflict-tooltips branch from 40e54ab to c761c0b Compare October 29, 2024 15:51
clarani
clarani reviewed Oct 31, 2024
View reviewed changes
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

  • The positions should be bold
  • Do you think you can replace the , by . for the positions to follow the mockup ?
    image

Mockup:
image

@emersion
ui-spacetimechart: store path ID in stories state
8dced2f 
We never need anything else from the HoveredItem, and this change
simplifies a future commit which adds a new PickingElement variant
without a pathId field.

Signed-off-by: Simon Ser <[email protected]>
@emersion
ui-spacetimechart: use switch in snapPosition()
494ad87 
Don't assume that a PickingElement is a segment if it's not a point.
This is useful for the next commit which introduces a new
PickingElement variant.

Signed-off-by: Simon Ser <[email protected]>
@emersion
ui-spacetimechart: add utility to draw a rect onto an ImageData
71992c6 
Useful to build interactive rectangles: the picking canvas needs
to be painted with hand-rolled operations on ImageData.

Signed-off-by: Simon Ser <[email protected]>
@emersion
ui-spacetimechart: add conflicts to picking canvas
4d9c584 
Signed-off-by: Simon Ser <[email protected]>
@emersion
ui-spacetimechart: add conflict tooltip
674e6e9 
Signed-off-by: Simon Ser <[email protected]>
@emersion emersion force-pushed the emr/conflict-tooltips branch from c761c0b to 674e6e9 Compare October 31, 2024 10:51
@emersion
Copy link
Member Author

Thanks for the detailed comments, many things I've missed! I'm not super sure all of these details were really intentional UX decisions, but oh well.

Do you think you can replace the , by . for the positions to follow the mockup ?

Ah yes, I've been formatting numbers according to the current locale, but it seems the expectation is to always use English-style floating point number formatting here… Fixed by forcing the locale to US.

clarani
clarani approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@emersion emersion enabled auto-merge October 31, 2024 12:03
@emersion emersion removed the request for review from Synar October 31, 2024 12:03
@emersion emersion added this pull request to the merge queue Oct 31, 2024
Merged via the queue into dev with commit f672fd7 Oct 31, 2024
6 checks passed
@emersion emersion deleted the emr/conflict-tooltips branch October 31, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Synar Synar Synar approved these changes

@clarani clarani clarani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spacetimechart: add conflicts tooltips
3 participants
@emersion @Synar @clarani