Skip to content

Add Fixed Time Zone Support for NowIndicator#174

Open
MasterHiei wants to merge 4 commits intoJonasWanke:mainfrom
MasterHiei:timezone-support
Open

Add Fixed Time Zone Support for NowIndicator#174
MasterHiei wants to merge 4 commits intoJonasWanke:mainfrom
MasterHiei:timezone-support

Conversation

@MasterHiei
Copy link
Contributor

Hi @JonasWanke 👋

First of all, thank you for your awesome package!

I encountered a situation where we needed to set the now indicator with a fixed time zone. To address this, I created this PR.

I also saw that similar functionality is WIP, but I’m hoping to use it as soon as possible, so I decided to go ahead with this solution for now.

Please feel free to review the changes, and I’d love to hear any feedback or suggestions!

Thanks again for your great work!

Copy link
Owner

@JonasWanke JonasWanke left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, and sorry for the delay! I agree that customizing the current time is a useful feature, but I'd go a bit differently about the implementation:

Instead of having users provide a time zone ID and explicitly requiring the timezone package, I'd just take a DateTime Function() getNow (or with a different name). In the documentation, we can still point to the timezone package, but a getter allows the flexibility to use a different package, using DateTime.now() and adding a fixed offset directly, or even set a fixed time for testing.

@MasterHiei
Copy link
Contributor Author

Thanks for your advice! I'm a bit busy right now, but I'll check it later :)

devicePixelRatio != oldDelegate.devicePixelRatio;

DateTime get _now =>
timeController.getCurrentTime?.call().copyWith(isUtc: true) ??
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, users should provide UTC time according to the package’s original design. However, since this isn’t common practice, handle the conversion internally looks better.

@MasterHiei
Copy link
Contributor Author

@JonasWanke Hi 👋
Apologies for the long delay—over 7 months! I’ve updated the PR according to your comment.
Please take a look when you get a chance, and I’d love to hear any further suggestions or ideas you might have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants