Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Time display mode settings #3688

Merged
merged 3 commits into from
Nov 11, 2020
Merged

Conversation

rahulbile
Copy link
Collaborator

@rahulbile rahulbile commented Nov 9, 2020

Description:

Currently, the time displayed in-app for transactions is in 12 hour mode, some users may prefer to change it to 24 hour display mode

Motivation and Context:

The feature request was requested in #3600 and while doing this change, the display of dates across the app is refactored to have more control on formats and making sure its extensible in future easily

Types of changes:

Feature

Checklist:

  • My code follows the code style of this project.
  • I have reviewed and updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes where needed.
  • All new and existing tests passed.
  • My commits have been squashed into a concise set of changes.

@welcome
Copy link

welcome bot commented Nov 9, 2020

⚡️ Thanks for opening this pull request! ⚡️
We use conventional commits to streamline the release process. Before your pull request can be merged, you should squash your commits and reword your commit messages to follow the conventional commit message format.
Examples of commit messages with semantic prefixes:

  • fix(ui): ensure search input is cleared after submit
  • feat(wallet): add new transaction detail view
  • perf(channels): prevent double render in channel list
    Things that will help get your PR across the finish line:
  • Run yarn lint locally to catch formatting errors earlier.
  • Run yarn test locally to catch test failures early.
  • Run yarn extract-messages locally to extract new transaction strings.
  • Document any user-facing changes you've made.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

@rahulbile rahulbile changed the title [WIP] Rb/time format 3600 Rb/time format 3600 Nov 9, 2020
@rahulbile rahulbile changed the title Rb/time format 3600 24 hour time format Nov 9, 2020
Copy link
Member

@mrfelton mrfelton left a comment

Choose a reason for hiding this comment

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

Nice one @rahulbile . A few comments/suggestions

renderer/components/Activity/PaymentModal/PaymentModal.js Outdated Show resolved Hide resolved
renderer/components/Activity/Row.js Outdated Show resolved Hide resolved
config/default.js Outdated Show resolved Hide resolved
renderer/components/Settings/messages.js Outdated Show resolved Hide resolved
renderer/containers/UI/FormattedDateTime.js Outdated Show resolved Hide resolved
renderer/containers/UI/FormattedDateTime.js Outdated Show resolved Hide resolved
@rahulbile rahulbile force-pushed the rb/time-format-3600 branch 5 times, most recently from 5778305 to 85e3d73 Compare November 9, 2020 14:39
@rahulbile rahulbile changed the title 24 hour time format Time display mode settings Nov 9, 2020
config/default.js Outdated Show resolved Hide resolved
@rahulbile rahulbile force-pushed the rb/time-format-3600 branch 3 times, most recently from a203b77 to fffda3f Compare November 9, 2020 15:58
Copy link
Member

@mrfelton mrfelton left a comment

Choose a reason for hiding this comment

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

Am seeing the following error in the console:

Warning: Failed prop type: Invalid prop value supplied to FormattedDateTime

Happens after sending an on chain transaction, whist it's in pending state:

It also seems to happen when the activity list first loads


const Row = ({ style, item, RowComponent }) => (
<div style={style}>
{item.title ? (
<Box mt={4} pl={4}>
<Heading.H4 fontWeight="normal">
<FormattedDate day="2-digit" month="short" value={item.title} year="numeric" />
<FormattedDateTime format="date" month="short" value={item.title} />
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is what's causing the prop warning? What is title? Is it always a string or Date?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah looks like in some cases its not a date, so have updated the value validation to be: PropTypes.any, will that be fine? Doing so the error does go away, shall we format the value before sending or any validation is fine?

Copy link
Member

Choose a reason for hiding this comment

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

I guess really the component supports dates passed as string (which is what item.title is). Probably should update component prop type definition to oneOfType([PropTypes.number, PropTypes.string, PropTypes.instanceOf(Date)])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, adding that works fine and also fixes the validation issue. Updated the PR

renderer/containers/UI/FormattedDateTime.js Outdated Show resolved Hide resolved
renderer/containers/UI/FormattedDateTime.js Outdated Show resolved Hide resolved
renderer/containers/UI/FormattedDateTime.js Outdated Show resolved Hide resolved
renderer/containers/UI/FormattedDateTime.js Outdated Show resolved Hide resolved
@rahulbile rahulbile force-pushed the rb/time-format-3600 branch 4 times, most recently from cbd19ee to 1ab88ed Compare November 10, 2020 18:09
Copy link
Member

@mrfelton mrfelton left a comment

Choose a reason for hiding this comment

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

Tested ACK aae3838

@mrfelton mrfelton merged commit 5d6047d into LN-Zap:next Nov 11, 2020
@welcome
Copy link

welcome bot commented Nov 11, 2020

Congrats on merging your first pull request! ⚡️⚡️⚡️

@mrfelton mrfelton mentioned this pull request Nov 11, 2020
@rahulbile rahulbile deleted the rb/time-format-3600 branch November 14, 2020 14:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants