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

Updated Unix Timestamp Converter #509

Merged
merged 28 commits into from
May 21, 2022
Merged

Updated Unix Timestamp Converter #509

merged 28 commits into from
May 21, 2022

Conversation

niyari
Copy link
Contributor

@niyari niyari commented Apr 30, 2022 •
edited
Loading

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Issue Number: #473

What is the new behavior?

image

  • Time zone support.
  • Support for time carry forward and backward.
  • Resolved performance degradation that occurs when values are out of range.

Support for time carry forward and backward.

image

Other information

  • Conflict with the addition of a Now button needs to be resolved.

Quality check

Before creating this PR, have you:

  • Followed the code style guideline as described in CONTRIBUTING.md
  • Verified that the change work in Release build configuration
  • Checked all unit tests pass

@niyari
Copy link
Contributor Author

niyari commented May 3, 2022

Looks good to display in text, InfoBar can be removed.
Timestamp to .NET Ticks (If features are to be added here.) #512
image

@niyari niyari marked this pull request as ready for review May 10, 2022 20:44
@veler
Copy link
Collaborator

veler commented May 14, 2022

Hello :)
Thank you for getting the PR up, and sorry for the late answer.

A few remarks on the screenshot you shared:

The following info is great, but they visually don't align with the rest of the UI on the left:

image

Suggestion:

What about putting this info either below the Time Zone or Year Month Day... input?
Additionally, perhaps it could be place into a Grid with a lighter background color, like through this style:

https://github.com/veler/DevToys/blob/b11f267ef738501bbf993c677013777cb764b1aa/src/dev/impl/DevToys/Views/Tools/Graphic/ColorBlindnessSimulator/ImagePreview.xaml#L12-L23

Which looks like this:

image

Or even, an expandable container, like in the setting page:

image

Here is how it's made:
https://github.com/veler/DevToys/blob/da659990b1185011edb8182ad340e37e6b5f6611/src/dev/impl/DevToys/Views/Tools/Settings/SettingsToolPage.xaml#L142-L165

What do you think? :)

Thank you !

@niyari
Copy link
Contributor Author

niyari commented May 14, 2022

Timezone info UI is derived from the Text Case Converter.
Of the suggestions for unifying visual information, we have 3 candidates.
expandable container hides much needed information for users living in areas that do not use daylight savings time.

image

In the case of A and B, B is usually more compact on the screen.
However, DevToys is expected to be used primarily with some kind of work.
In order for DevToys to be a viable tool with the smallest screen size, all the operable items need to be gathered at the top.

image

It would be best if the screen layout could be changed according to the window size like on the web, but first we would like to display it at the bottom of the C plan.
image

@veler
Copy link
Collaborator

veler commented May 14, 2022

Hello,

Thanks for your quick answer :)

I like A plan better!
Although you are right, either plan A, B or C, there is an issue with Compact Mode.
Good thing is, by using VisualState, we can actually change the layout on Compact Mode. We do something like this for TextDiff tool:
https://github.com/veler/DevToys/blob/b11f267ef738501bbf993c677013777cb764b1aa/src/dev/impl/DevToys/Views/Tools/Text/TextDiff/TextDiffToolPage.xaml#L14-L30

We hide the input fields and for this, we have to change a bit the layout so it resize accordingly. Perhaps we can do the same here so some information displayed horizontally turn to be displayed vertically?

@niyari
Revert to e45d522.
b2807f7 
 (Timezone info: 509#issuecomment-1126643640 )
@niyari
Copy link
Contributor Author

niyari commented May 14, 2022

Responsive (changes with window width) is going to be better regardless of mode.
I would like to make this a separate PR, what do you think?

Furthermore, I found that after changing to compact mode, changing the window size causes the application to stop.
I will submit a new issue.

@veler
Copy link
Collaborator

veler commented May 14, 2022

Hello,
Thanks for your answer. I'm fine with addressing that in another PR, perhaps I can even do that myself later.
There are some merge conflict in this PR apparently :)

@niyari
Copy link
Contributor Author

niyari commented May 14, 2022

How about this?(use AdaptiveTrigger)
image

niyari added 3 commits May 15, 2022 03:06
@niyari
Changed UI.
1571ed6 
Content display by window width.
@niyari
Preparation for Conflict Resolution.
41c29e6
@niyari
Merge branch 'dev-now' into chrono
5ab5e22 
# Conflicts:
#	src/dev/impl/DevToys/ViewModels/Tools/Converters/Timestamp/TimestampToolViewModel.cs
#	src/dev/impl/DevToys/Views/Tools/Converters/Timestamp/TimestampToolPage.xaml
@veler
Copy link
Collaborator

veler commented May 15, 2022

How about this?(use AdaptiveTrigger) image

I like that better! In particular the year/month/day going on a second line.
I find a bit more weird, however, that the time zone information is jumping from above year/month/day to below year/month/day.

@niyari
Copy link
Contributor Author

niyari commented May 16, 2022

I think this will work.

@veler
Copy link
Collaborator

veler commented May 16, 2022

Can we keep the TimeZone information just below the TimeZone combo box?

@niyari
Copy link
Contributor Author

niyari commented May 16, 2022

In commit c3027e0, the Timezone combo box is back to fixed.
image

veler
veler requested changes May 17, 2022
View reviewed changes
Copy link
Collaborator

@veler veler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I looked at the PR, built the branch and played with it. It looks very good overall 😁 Good job! Just a few details and here and there and we can merge this awesome PR! 😁

src/dev/impl/DevToys/Views/Tools/Converters/Timestamp/TimestampToolPage.xaml Outdated Show resolved Hide resolved
src/dev/impl/DevToys/Views/Tools/Converters/Timestamp/TimestampToolPage.xaml Show resolved Hide resolved
src/dev/impl/DevToys/Views/Tools/Converters/Timestamp/TimestampToolPage.xaml Outdated Show resolved Hide resolved
src/dev/impl/DevToys/Views/Tools/Converters/Timestamp/TimestampToolPage.xaml.cs Outdated Show resolved Hide resolved
src/dev/impl/DevToys/ViewModels/Tools/Converters/Timestamp/TimestampToolViewModel.cs Outdated Show resolved Hide resolved
src/dev/impl/DevToys/ViewModels/Tools/Converters/Timestamp/TimestampToolViewModel.cs Outdated Show resolved Hide resolved
src/dev/impl/DevToys/ViewModels/Tools/Converters/Timestamp/TimestampToolViewModel.cs Outdated Show resolved Hide resolved
src/dev/impl/DevToys/Helpers/TimestampToolHelper.cs Outdated Show resolved Hide resolved
@veler veler linked an issue May 17, 2022 that may be closed by this pull request
Unix Timestamp Converter - Timezone select #473
Closed
veler
veler approved these changes May 21, 2022
View reviewed changes
Copy link
Collaborator

@veler veler left a comment

Choose a reason for hiding this comment

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

It looks good now! :D Thank you very much for this great pull request! I appreciate your contribution a lot! :D

@veler veler merged commit 4b6fc5c into DevToys-app:main May 21, 2022
@veler veler removed a link to an issue May 21, 2022
Unix Timestamp Converter - Timezone select #473
Closed
@veler veler linked an issue May 21, 2022 that may be closed by this pull request
Unix Timestamp Converter - Timezone select #473
Closed
@niyari niyari deleted the chrono branch June 2, 2022 21:08
veler added a commit that referenced this pull request Mar 31, 2023
@niyari @veler
Updated Unix Timestamp Converter (#509)
ad1a278 
* Add string keywords.

* Update TimestampTool

* Add info text.

* Update UI

* Add string keywords. (/*/Timestamp.resw)

* Fix UI

* Add TimestampToolHelper.

* Update TimestampTool

* Update Timestamp.resw

* Cleanup file.

* Added CardStyle.

* Stack UI

* Fix UI

* Revert to e45d522.

 (Timezone info: 509#issuecomment-1126643640 )

* Changed UI.

Content display by window width.

* Preparation for Conflict Resolution.

* Removed compact DateTime info.

* Code cleanup #discussion_r874407747

* Changed the clipboard datetime string recognition process.

* Added summary.

* Fix types.

* Fixed Page.Resources.

* Change names.

* Remove Strings.CurrentDateTimeTitle

* Change names.

* Added comment.

Co-authored-by: Etienne BAUDOUX <ebaudoux@velersoftware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Unix Timestamp Converter - Timezone select
2 participants
@niyari @veler