-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this 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]), |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 |
---|---|
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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? 🙂
There was a problem hiding this comment.
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.
- Intervals like
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some examples:
There was a problem hiding this comment.
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.
015144a
to
7fc33a6
Compare
- Split only when the interval exceeds 5am of the next day - Display midnight and 24/7 better to the user
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
//edit @zbycz: added examples:
Examples
https://osmapp-git-fork-dlurak-opening-hours-enhancements-osm-app-team.vercel.app/node/10240501216
way/649565814
node/7101244895
node/11392853254
(note for myself: i used search
op:node["opening_hours"~"-2:"]
)