Skip to content

fix(datetime): datetime locale with h23 will respect max time range #24610

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

Merged
sean-perkins merged 1 commit into from
Jan 20, 2022
Merged

fix(datetime): datetime locale with h23 will respect max time range #24610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions core/src/components/datetime/test/data.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,5 +257,35 @@ describe('generateTime()', () => {
expect(minutes.length).toEqual(60);
});
});

it('should respect the min & max bounds', () => {
const refValue = {
day: undefined,
month: undefined,
year: undefined,
hour: 20,
minute: 30
}

const minParts = {
day: undefined,
month: undefined,
year: undefined,
hour: 19,
minute: 30
}

const maxParts = {
day: undefined,
month: undefined,
year: undefined,
hour: 20,
minute: 40
};

const { hours } = generateTime(refValue, 'h23', minParts, maxParts);

expect(hours).toStrictEqual([19, 20]);
});
})
})
142 changes: 65 additions & 77 deletions core/src/components/datetime/test/minmax/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,89 +1,77 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8">
<title>Datetime - Min/Max</title>
<meta name="viewport"
content="width=device-width, initial-scale=1.0, minimum-scale=1.0">
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet">
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet">
<script src="../../../../../scripts/testing/scripts.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
<style>
.grid {
display: grid;
grid-template-columns: repeat(2, minmax(250px, 1fr));
grid-gap: 20px;
}
h2 {
font-size: 12px;
font-weight: normal;

color: #6f7378;
<head>
<meta charset="UTF-8">
<title>Datetime - Min/Max</title>
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0">
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet">
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet">
<script src="../../../../../scripts/testing/scripts.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
<style>
.grid {
display: grid;
grid-template-columns: repeat(2, minmax(250px, 1fr));
grid-gap: 20px;
}

margin-top: 10px;
margin-left: 5px;
}
h2 {
font-size: 12px;
font-weight: normal;

ion-datetime {
box-shadow: 0px 16px 32px rgba(0, 0, 0, 0.25), 0px 8px 16px rgba(0, 0, 0, 0.25);
border-radius: 8px;
}
</style>
</head>
<body>
<ion-app>
<ion-header translucent="true">
<ion-toolbar>
<ion-title>Datetime - Min/Max</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding">
<div class="grid">
<div class="grid-item">
<h2>Value inside Bounds</h2>
<ion-datetime
id="inside"
min="2021-09"
max="2021-10"
></ion-datetime>
</div>
<div class="grid-item">
<h2>Value Outside Bounds</h2>
<ion-datetime
id="outside"
min="2021-06-05"
max="2021-06-19"
value="2021-06-20"
></ion-datetime>
</div>
<div class="grid-item">
<h2>AM/PM Min/Max</h2>
<ion-datetime
presentation="time"
min="09:30"
max="14:50"
value="10:30"
></ion-datetime>
</div>
<div class="grid-item">
<h2>h23 Min</h2>
<ion-datetime
presentation="time"
hour-cycle="h23"
min="19:30"
value="20:30"
></ion-datetime>
</div>
color: #6f7378;

margin-top: 10px;
margin-left: 5px;
}

ion-datetime {
box-shadow: 0px 16px 32px rgba(0, 0, 0, 0.25), 0px 8px 16px rgba(0, 0, 0, 0.25);
border-radius: 8px;
}
</style>
</head>

<body>
<ion-app>
<ion-header translucent="true">
<ion-toolbar>
<ion-title>Datetime - Min/Max</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding">
<div class="grid">
<div class="grid-item">
<h2>Value inside Bounds</h2>
<ion-datetime id="inside" min="2021-09" max="2021-10"></ion-datetime>
</div>
<div class="grid-item">
<h2>Value Outside Bounds</h2>
<ion-datetime id="outside" min="2021-06-05" max="2021-06-19" value="2021-06-20"></ion-datetime>
</div>
</ion-content>
<div class="grid-item">
<h2>AM/PM Min/Max</h2>
<ion-datetime presentation="time" min="09:30" max="14:50" value="10:30"></ion-datetime>
</div>
<div class="grid-item">
<h2>h23 Min</h2>
<ion-datetime presentation="time" hour-cycle="h23" min="19:30" value="20:30"></ion-datetime>
</div>
<div class="grid-item">
<h2>locale: en-GB</h2>
<ion-datetime locale="en-GB" presentation="time" min="19:30" value="20:30" max="20:40"></ion-datetime>
</div>
</div>
</ion-content>

<script>
<script>
const datetime = document.querySelector('ion-datetime');
datetime.addEventListener('ionChange', (ev) => {
console.log('Change', ev.detail.value);
})
</script>
</ion-app>
</body>
</script>
</ion-app>
</body>

</html>
2 changes: 1 addition & 1 deletion core/src/components/datetime/utils/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ export const generateTime = (
if (maxParts.hour !== undefined) {
processedHours = processedHours.filter(hour => {
const convertedHour = refParts.ampm === 'pm' ? (hour + 12) % 24 : hour;
Copy link
Contributor

@liamdebeasi liamdebeasi Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain why this change works? If you are using 24 hour time, refParts.ampm should be undefined, which means convertedHour would be hour.

Copy link
Contributor Author

@sean-perkins sean-perkins Jan 20, 2022

Choose a reason for hiding this comment

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

The workingParts (used as refParts in that util function) has ampm defined. Likely as a result of when we call processValue on initialization of the datetime component:

...
this.workingParts = {
  ...
  ampm: hour >= 12 ? 'pm' : 'am'
}

Where as the hour cycle only takes into consideration the locale or if the developer has hard-set the hour cycle.

So this case exists when the developer hasn't specified the hour cycle manually, but the locale requires it.

Copy link
Contributor

@liamdebeasi liamdebeasi Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok thanks. Would it make more sense to not have ampm be optional then? https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime-interface.ts#L21

Copy link
Contributor Author

@sean-perkins sean-perkins Jan 20, 2022

Choose a reason for hiding this comment

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

Yeah, I think that makes sense for now. I'll confirm we aren't using that interface for any partial date object comparisons.

Copy link
Contributor Author

@sean-perkins sean-perkins Jan 20, 2022

Choose a reason for hiding this comment

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

Looks like we are in a few places:

I'll create a tech debt task to have more strict explicit types.

return convertedHour <= maxParts.hour!;
return (use24Hour ? hour : convertedHour) <= maxParts.hour!;
});
isPMAllowed = maxParts.hour >= 13;
}
Expand Down