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.

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

Implement new date/time range picker #116

Merged
merged 11 commits into from
Jun 9, 2024
Merged

Conversation

jduar
Copy link
Contributor

@jduar jduar commented Jun 9, 2024

Hey! Thanks for this frontend. It's been of great use to me but I've been having some trouble setting dates, especially on mobile (the behaviour described on #43) , so I took it upon myself to try and get the changes proposed by @Tofee merged.

This PR picks up on @Tofee 's work started in his PR (#88). I've resolved the existing merge conflicts and made a few design changes so the datepicker fits more within the style used in owntracks/frontend and also to improve mobile behaviour as a whole. Also made a few changes so the date range is always shown in both desktop and mobile. I've made a few tests in different device resolutions and things seem okay now.

I added the shortcuts you can see in the screenshots below (today, 7 days, etc.) that allow filtering by those default dates without having to insert them - I often find myself wanting to see where I've been in the present day or in the last week. Also added a button so one can go from date choice to time choice without having to change either.

We decided to throw this into a new PR as it seemed the cleaner way to go about it.

I'll leave a few screenshots below.

desktop2
desktop1
mobile3
mobile2
mobile1

Feel free to make any suggestion or request any change. I feel these changes very much improve mobile date picking behaviour and I'd much like to see them merged. Thanks!


Closes #43

@linusg
Copy link
Member

linusg commented Jun 9, 2024

Thank you very much for reviving this! Two requests:

  • Can we add some styles to remove the calendar icon within the date range button? There already is one before it so that seems redundant.
  • I seem to have created some conflicts by removing the download modal just now, could you rebase this?

src/components/AppHeader.vue Outdated Show resolved Hide resolved
src/components/AppHeader.vue Outdated Show resolved Hide resolved
src/components/AppHeader.vue Outdated Show resolved Hide resolved
src/components/AppHeader.vue Outdated Show resolved Hide resolved
@jduar
Copy link
Contributor Author

jduar commented Jun 9, 2024

Removing the calendar icon actually leaves more room for the text so it doesn't look as cramped. The dash works well to visually separate dates IMO.

postmobile1
postdesktop1

@linusg
Copy link
Member

linusg commented Jun 9, 2024

Thanks, tried this locally and it looks great! I'd like to suggest a few more changes, could you apply this diff?

diff --git a/src/components/AppHeader.vue b/src/components/AppHeader.vue
index 56881e8..8d4114f 100644
--- a/src/components/AppHeader.vue
+++ b/src/components/AppHeader.vue
@@ -54,16 +54,19 @@
       <div class="nav-item">
         <CalendarIcon size="1x" aria-hidden="true" role="img" />
         <date-picker
-          v-model="rangeDateTime"
+          v-model="dateTimeRange"
           type="datetime"
           format="YYYY-MM-DD HH:mm"
-          range
-          range-separator=" – "
-          :show-second="false"
+          :editable="false"
+          :clearable="false"
           :confirm="true"
+          :show-second="false"
+          :range="true"
+          range-separator=" – "
           :shortcuts="shortcuts"
           :show-time-panel="showTimeRangePanel"
-          @change="handleChange"
+          :disabled-date="(date, _) => date > new Date()"
+          @change="handleDateTimeRangeChange"
         >
           <template v-slot:footer>
             <button
@@ -193,6 +196,7 @@ export default {
           text: this.$t("Today"),
           onClick() {
             const end = new Date();
+            end.setHours(23, 59, 59, 0);
             const start = new Date();
             start.setHours(0, 0, 0, 0);
             return [start, end];
@@ -202,8 +206,10 @@ export default {
           text: this.$t("7 days"),
           onClick() {
             const end = new Date();
+            end.setHours(23, 59, 59, 0);
             const start = new Date();
             start.setDate(end.getDate() - 7);
+            start.setHours(0, 0, 0, 0);
             return [start, end];
           },
         },
@@ -211,8 +217,10 @@ export default {
           text: this.$t("30 days"),
           onClick() {
             const end = new Date();
+            end.setHours(23, 59, 59, 0);
             const start = new Date();
             start.setDate(end.getDate() - 30);
+            start.setHours(0, 0, 0, 0);
             return [start, end];
           },
         },
@@ -245,7 +253,7 @@ export default {
         this.setSelectedDevice(value);
       },
     },
-    rangeDateTime: {
+    dateTimeRange: {
       get() {
         const startDateTime = moment
           .utc(this.$store.state.startDateTime, DATE_TIME_FORMAT)
@@ -283,7 +291,7 @@ export default {
       this.showTimeRangePanel = !this.showTimeRangePanel;
     },
     // Resetting to date choice after value change
-    handleChange(value, type) {
+    handleDateTimeRangeChange(value, type) {
       this.showTimeRangePanel = false;
     },
   },
diff --git a/src/styles/_base.scss b/src/styles/_base.scss
index 77f8c8b..1b0ec2c 100644
--- a/src/styles/_base.scss
+++ b/src/styles/_base.scss
@@ -101,8 +101,7 @@ pre {
   min-height: 100%;
   flex-direction: column;
 
-  // Only select immediate child as the datepicker contains a <header> as well
-  > header {
+  header {
     display: flex;
     padding: 20px;
     white-space: nowrap;
diff --git a/src/styles/_datepicker.scss b/src/styles/_datepicker.scss
index 3ff976b..4cc6121 100644
--- a/src/styles/_datepicker.scss
+++ b/src/styles/_datepicker.scss
@@ -1,5 +1,5 @@
 .mx-datepicker {
-  width: 330px;
+  width: 280px;
 
   .mx-input {
     border: 0;
@@ -40,7 +40,6 @@
   margin-right: 10px;
 }
 
-.mx-icon-calendar,
-.mx-icon-clear {
-  visibility: hidden;
+.mx-icon-calendar {
+  display: none;
 }

@linusg
Copy link
Member

linusg commented Jun 9, 2024

Note that since you have a monospace font override, the sizing is different - with the default font it would look like this, so I reduced the width:

image

@jduar
Copy link
Contributor Author

jduar commented Jun 9, 2024

I'll apply these changes and get back to you. In the mean time, maybe you can help me with something?

After merging the latest changes from main running npm install on a clean install without node_modules is resulting in eslint-config-prettier not being installed, which then means npm run lint:js fails to run (see https://github.com/jduar/owntracks-frontend/actions/runs/9439288457/job/25997235566). Am I doing something wrong or is eslint-config-prettier supposed to be a dependency? Could this be due to a mismatch between our local npm versions?

@linusg
Copy link
Member

linusg commented Jun 9, 2024

Your version of the lockfile seems to remove it for some reason:

image

I'd recommend restoring the lockfile from the main branch, then running npm remove vue-ctk-date-time-picker and npm add --save vue2-datepicker, and committing the result of that. In my case eslint-config-prettier is preserved after that, please try if it works for you as well :)

@jduar
Copy link
Contributor Author

jduar commented Jun 9, 2024

That worked, thanks!

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Alright, seems good! Thanks again :)

@linusg linusg changed the title New range datepicker Implement new date/time range picker Jun 9, 2024
@linusg linusg merged commit b141444 into owntracks:main Jun 9, 2024
1 check passed
@linusg
Copy link
Member

linusg commented Jun 9, 2024

Went ahead and tagged another release to avoid accumulating changes again: https://github.com/owntracks/frontend/releases/tag/v2.14.0

@ everyone: Please test this in prod and report any issues you may find!

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.

Update date/time picker to only load data on confirmation
3 participants