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

FeaturePanel: Enhancements to the opening hours #516

Merged
merged 6 commits into from
Sep 8, 2024

Conversation

Dlurak
Copy link
Collaborator

@Dlurak Dlurak commented Sep 5, 2024

Description

This PR addresses the issue of incorrect display of opening hours that extend beyond midnight, such as with "24/7" businesses. To handle this, the code now internally divides the opening intervals into segments that do not exceed midnight. This change ensures that opening hours are accurately represented in all cases.
As discussed here I also added strings to indicate that a feature opens/closes soon. I'm not sure if it would be better to show "Opens in x minutes" instead of "Opens soon"; what do you think?

Checklist

  • checked dark mode / light mode
  • checked mobile / desktop
  • checked server-side-rendering (SSR)
  • code style is consistent with the rest of the project
  • new dependencies are reasoned about in PR comments

//edit @zbycz: added examples:

Examples

(note for myself: i used search op:node["opening_hours"~"-2:"])

Copy link

vercel bot commented Sep 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
osmapp ✅ Ready (Inspect) Visit Preview Sep 8, 2024 2:51pm

zbycz
zbycz previously requested changes Sep 7, 2024
Copy link
Owner

@zbycz zbycz left a comment

Choose a reason for hiding this comment

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

Thanks, "Closes soon" is definitely useful. See my comments about the other change. I Request change for the "const enums", otherwise it is up to discussion 🙂

@@ -36,8 +55,12 @@ export const parseComplexOpeningHours = (
oneWeekLater.setDate(oneWeekLater.getDate() + 7);

const intervals = oh.getOpenIntervals(today, oneWeekLater);
const splittedIntervals = intervals.flatMap(([openingDate, endDate]) =>
splitDateRangeAtMidnight([openingDate, endDate]),
Copy link
Owner

@zbycz zbycz Sep 7, 2024

Choose a reason for hiding this comment

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

ad splitting interval at midnight) Are you really sure, it is better? It feels natural for me to read an opening hour like Monday 17-02, I think it is clear that is over the midnight. Why did you really choose to split it into two?

I would guess the reason could be to compute the "closes soon" more properly? If this is the case, I would propose to still show the "human version" and just compute the opening by the split time.
Maybe you have some other reason, if so, please tell 🙂

I added some screenshots to your opening post. Especially in English it is in my opinion unreadable. (6PM - 2PM would be fine.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting view on it, which I agree with for your examples!
I primarily thought about cases where one interval starts at one day and ends at least two days later (e.g. 24/7). Previously it would show only one day as opened and all the other ones as closed - now each day gets exactly what's correct for the day.

Old New
Old New

Copy link
Owner

Choose a reason for hiding this comment

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

I see. Thats why you decided to split it by midnight. Now it makes sense 🙂 Please if possible, (next time) try to add more examples to the PR description, there is so much variation in OSM data.

So – I agree, that your fix to the 24/7 hours is defintely useful, though I still think you should not change the 18-02 to the split version. Do you agree? Can you find a way how to code it? If you like, we may discuss it over discord/videocall 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

Also one another idea – the 00-00 is rather unclear to me. It was like this before, so feel free to leave it alone, or leave it to a followup. Though, when we are about to show 00-00, I would strongly prefer 00-24 – what do you think? 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Formatting Enhancements

  • Added German and English translations for "midnight" (i.e., 24/12 PM).
  • Added German and English translations for "all day" (i.e., 24 hours).

Splitting Logic Update

  • The system now only splits intervals if they extend beyond 5 AM on the following day. This means that:
    • Intervals like 24/7 will be split.
    • Intervals like 18-02 will remain unchanged.

I chose 5 AM as the split point to balance clarity and usability. It ensures that intervals that extend slightly into the next day remain unaltered while making it clear when an interval overlaps into the morning or midday.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks. Translation of 00-00 to 24 hours is a good solution.

Though, please do not translate midnight in other intervals. When it is 18-00, it is clear already, and adding this to translations will brake all the languages that are not translated yet (becaues english would be used as default). What is even worse is, that the user can change locale to the other one, and it is not really 1:1 to the language then 😅

After you fix this, feel free to merge it.

btw, I prefixed the PR title with "FeaturePanel:" – please try to add the folder / bigger component where is your PR located. It helps me creating changelogs.

- Split only when the interval exceeds 5am of the next day
- Display midnight and 24/7 better to the user
@zbycz zbycz changed the title Enhancements to the opening hours FeaturePanel: Enhancements to the opening hours Sep 8, 2024
@zbycz zbycz dismissed their stale review September 8, 2024 14:25

requested change is resolved

@Dlurak Dlurak merged commit 5505fe4 into zbycz:master Sep 8, 2024
2 checks passed
@Dlurak Dlurak deleted the opening-hours-enhancements branch September 30, 2024 22:37
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.

2 participants