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

3148: Show link to make appointment even if no opening hours set #3168

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hannaseithe
Copy link
Contributor

@hannaseithe hannaseithe commented Mar 25, 2025

Short Description

The link to make an appointment will be shown regardless if Opening Hours for the POI are set or not.

Proposed Changes

  • This bug in native occured, where the OpeningHours Component (which the link is part of) was not rendered at all if no Opening Hours were set or if the POI was set as TemporarilyClosed (in that case only a stub of component was rendered without the link).
  • Now the link will be shown underneath the Opening Hours (inside that section) but not as part of the collapsible that contains the Opening Hours
  • Below I attached some pictures that show the new setup (Sorry for those giant pictures, couldnt figure out if there is a way to make them smaller on github)

Side Effects

Testing

  • Check whether the link to make an appointment is shown for POI
    - with link and opening hours
    - with link and no opening hours
    - with link & opening hours, but temporarily closed
  • Check whether the POI still looks ok for:
    • if there is no link but opening hours
    • if there is no link and no opening hours

Resolved Issues

Fixes: #3148


@hannaseithe
Copy link
Contributor Author

hannaseithe commented Mar 25, 2025

@hannaseithe hannaseithe marked this pull request as ready for review March 25, 2025 12:05
@hannaseithe
Copy link
Contributor Author

I am also tagging you, @hauf-toni here, to make sure that the UX approach is ok with you

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Nice work! I think if there are no opening hours and the poi is not temporarily closed, context for the appointment link is missing. I think it would be better if we could display the title Öffnungszeiten normally (perhaps with a new status Nur nach Terminvereinbarung or similar. What do you think (also @hauf-toni)?
The following is only an example:

If you decide to do this, this should also be done for web. Its totally fine to move this to a separate issue though as well.

</Content>
</Collapsible>
<HorizontalLine />
{isTemporarilyClosed && <TitleContainer language={language}>{openingHoursTitle}</TitleContainer>}
Copy link
Member

@steffenkleinle steffenkleinle Mar 26, 2025

Choose a reason for hiding this comment

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

🔧 This wraps the TitleContainer from openingHoursTitle with another TitleContainer. Is that necessary?

Suggested change
{isTemporarilyClosed && <TitleContainer language={language}>{openingHoursTitle}</TitleContainer>}
{isTemporarilyClosed && <TitleContainer language={language}>{openingHoursTitle}</TitleContainer>}

Edit: I just saw that you only moved this and this was the case before already. Perhaps you could still adjust this?

Comment on lines +100 to +101
{isTemporarilyClosed && <TitleContainer language={language}>{openingHoursTitle}</TitleContainer>}
{shouldShowCollapsible && (
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Hmm, I think the logic here is harder to understand than before. What do you think about keeping the logic as before but instead rendering the appointment link (+ extra lines) in multiple places?
E.g. something like

  const appointmentLink = appointmentUrl ? (
    <LinkContainer onPress={() => openExternalUrl(appointmentUrl, showSnackbar)} role='link'>
      <Link>{t('makeAppointment')}</Link>
      <StyledIcon Icon={ExternalLinkIcon} />
    </LinkContainer>
  ) : null

  if (isTemporarilyClosed || appointmentLink) {
    return (
      <>
        <TitleContainer language={language}>{openingHoursTitle}</TitleContainer>
        {appointmentLink}
        <HorizontalLine />
      </>
    )
  }

  if (!openingHours || openingHours.length !== weekdays.length) {
    return null
  }

  return <Collapsible>...</Collapsible>

Edit: This would also solve that Make an appointment has a context (Öffnungszeiten), see other comment.

issue_key: 3148
show_in_stores: true
platforms:
- native
Copy link
Member

Choose a reason for hiding this comment

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

native is sadly not a valid value here, this will break future releases:

Suggested change
- native
- android
- ios

Comment on lines +5 to +6
en: Link to make an appointment will now show even if opening hours are not set
de: Der Link um einen Termin zu vereinbaren wird angezeigt selbst wenn es keine Öffnungszeiten gibt
Copy link
Member

Choose a reason for hiding this comment

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

🙃

Suggested change
en: Link to make an appointment will now show even if opening hours are not set
de: Der Link um einen Termin zu vereinbaren wird angezeigt selbst wenn es keine Öffnungszeiten gibt
en: Fix missing link to make appointments.
de: Fehlende Links zum Vereinbaren von Terminen werden nun angezeigt.

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

See release note

@steffenkleinle
Copy link
Member

Perhaps you could also add a separator for web? https://webnext.integreat.app/testumgebung/de/locations/ort-mit-link-f%C3%BCr-termine

@hauf-toni
Copy link

Nice work! I think if there are no opening hours and the poi is not temporarily closed, context for the appointment link is missing. I think it would be better if we could display the title Öffnungszeiten normally (perhaps with a new status Nur nach Terminvereinbarung or similar. What do you think (also @hauf-toni)? The following is only an example:

If you decide to do this, this should also be done for web. Its totally fine to move this to a separate issue though as well.

thanks, I agree! without that context, users might assume the POI is closed and won’t even try to schedule an appointment. displaying the title Öffnungszeiten with a status like Nur nach Terminvereinbarung sounds like a great solution.

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.

Link to make an appointment at a POI missing in native
3 participants