Skip to content

date-picker: add am/pm mode in time selector#8620

Merged
Leopoldthecoder merged 8 commits intoElemeFE:devfrom
firesh:dev
Dec 4, 2017
Merged

date-picker: add am/pm mode in time selector#8620
Leopoldthecoder merged 8 commits intoElemeFE:devfrom
firesh:dev

Conversation

@firesh
Copy link
Copy Markdown
Contributor

@firesh firesh commented Dec 1, 2017

Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow Element's contributing guide (中文 | English | Español).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer relative issues for you PR.

#8270

@Leopoldthecoder
Copy link
Copy Markdown
Contributor

@firesh Thanks for contribution!

Could you add some test cases for this feature?

@firesh
Copy link
Copy Markdown
Contributor Author

firesh commented Dec 3, 2017

I will.

@firesh
Copy link
Copy Markdown
Contributor Author

firesh commented Dec 3, 2017

@Leopoldthecoder

if (!shouldShowAmPm) return '';
let isCapital = this.amPmMode === 'A';
let content = ' ';
if (shouldShowAmPm) content = content.concat((hour < 12) ? 'am' : 'pm');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here if (shouldShowAmPm) is not necessary since the method returns if !shouldShowAmPm. And content = content.concat((hour < 12) ? 'am' : 'pm'); can move to the previous line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Revised.

:class="{ 'active': hour === hours, 'disabled': hoursList[hour] }"
v-for="hour in arrowHourList">
{{ hour === undefined ? '' : ('0' + hour).slice(-2) }}
{{ ('0' + (amPmMode ? (hour % 12 || 12) : hour )).slice(-2) }}{{amPm(hour)}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should handle hour === undefined here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for it. Revised.

@Leopoldthecoder Leopoldthecoder merged commit f5aa24e into ElemeFE:dev Dec 4, 2017
if (!shouldShowAmPm) return '';
let isCapital = this.amPmMode === 'A';
let content = (hour < 12) ? ' am' : ' pm';
if (isCapital) content = content.toUpperCase();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it a good idea to change isCapital to isUpperCase to match the JavaScript function called toUpperCase()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine to use === 'A', because amPmMode only have 2 options: ['a', 'A'].

@kcpal-qode
Copy link
Copy Markdown

kcpal-qode commented Feb 5, 2018

Hello @firesh ,

How can I add the name of the input elements for both the time picker in time range? Currently if I am trying to use time range with name="field_name", then it's only taking the first two letter of the name given.For example: In this case, 'f' for first timepicker in range" and 'i' for second timepicker in range.... So, how are we going to solve it?

@wacky6
Copy link
Copy Markdown
Contributor

wacky6 commented Feb 5, 2018

@kcpal-qode Use an array. https://jsfiddle.net/rz9b603h/

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.

5 participants