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

Highlight clock when out of sync #97

Merged
merged 1 commit into from
Feb 21, 2023
Merged

Highlight clock when out of sync #97

merged 1 commit into from
Feb 21, 2023

Conversation

mnishiguchi
Copy link
Member

@mnishiguchi mnishiguchi commented Oct 30, 2022 •
edited
Loading

resolves #74

CleanShot 2022-10-30 at 19 19 16@2x

TODO

  • resolve merge conflict
  • add (unsynchronized)
  • yellow text instead of yellow background

jjcarstens
jjcarstens reviewed Nov 30, 2022
View reviewed changes
Copy link
Member

@jjcarstens jjcarstens left a comment

Choose a reason for hiding this comment

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

I have a couple thoughts here:

  • Is changing color enough of a context? I'm not sure how easy it is to deduce that yellow == unsynchronized. Maybe we need to include (unsynchronized) next to it as well?
  • I'm going back and forth on having yellow background vs yellow text. It seems like yellow text might be less aggressive?

@mnishiguchi mnishiguchi force-pushed the mnishiguchi/time-out-of-sync branch from 992deb5 to 1e4a5b7 Compare December 2, 2022 01:01
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/time-out-of-sync branch from 1e4a5b7 to f81d0a4 Compare February 19, 2023 03:56
fhunleth
fhunleth approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@fhunleth fhunleth left a comment

Choose a reason for hiding this comment

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

Looks good.

@fhunleth
Copy link
Member

I missed that CI is currently broke. It will look good after that's fixed. Also, could you update CI to test with Elixir 1.14.3? Elixir 1.9 and 1.10 and be removed, since other packages require at least 1.11 anyway.

@mnishiguchi mnishiguchi force-pushed the mnishiguchi/time-out-of-sync branch from 77ce7fb to 8318380 Compare February 21, 2023 00:45
@mnishiguchi
Copy link
Member Author

✅ quick test on a mangopi_mq_pro as of 8318380

  • for a few minutes after the boot, the clock is out of synch and then it gets synched

@mnishiguchi mnishiguchi force-pushed the mnishiguchi/time-out-of-sync branch from 8318380 to 1572845 Compare February 21, 2023 01:28
@mnishiguchi
Copy link
Member Author

I missed that CI is currently broke. It will look good after that's fixed. Also, could you update CI to test with Elixir 1.14.3? Elixir 1.9 and 1.10 and be removed, since other packages require at least 1.11 anyway.

I'll update the CI in a separate PR.

@mnishiguchi mnishiguchi merged commit 446ed05 into main Feb 21, 2023
@mnishiguchi mnishiguchi deleted the mnishiguchi/time-out-of-sync branch February 21, 2023 01:40
@mnishiguchi mnishiguchi mentioned this pull request Feb 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjcarstens jjcarstens jjcarstens left review comments

@fhunleth fhunleth fhunleth 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.

Highlight time if not synchronized
3 participants
@mnishiguchi @fhunleth @jjcarstens