DDFFORM 417 #568
DDFFORM 417 #568
Conversation
| // These types are based on the actual data structure returned from the API. | ||
| // The types starting with DplOpeningHours.. are generated with Orval in react | ||
| // and copy pasted here. | ||
| export type DplOpeningHoursListGET200Item = { |
There was a problem hiding this comment.
👍 Appreciate the commenting here.
Normally I would not think that these "technicalities" belonged in the design system"
| <div className="opening-hours"> | ||
| <div className="opening-hours__wrapper"> | ||
| <div className="opening-hours__header"> | ||
| <h2 className="opening-hours__heading">Åbningstider</h2> | ||
| <div className="opening-hours__navigation-controls"> | ||
| <button | ||
| className="opening-hours__navigation-control" | ||
| aria-label="Previous week" | ||
| type="button" | ||
| > | ||
| <ArrowLeft /> | ||
| </button> | ||
| <div className="opening-hours__week-display"> | ||
| {weekCurrentlyDisplayed} | ||
| </div> | ||
| <button | ||
| className="opening-hours__navigation-control" | ||
| aria-label="Next week" | ||
| type="button" | ||
| > | ||
| <ArrowRight /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <OpeningHoursWeekList groupedOpeningHours={groupedOpeningHours} /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Consider if we can drop the div className="opening-hours__wrapper"> element.
We already have a <div className="opening-hours"> element to wrap the entire component.
There was a problem hiding this comment.
I need the layout-container for opening_hours, but the contents need to be max-width:600px. within that.
I wasn't sure whether to use a wrapper for all the content, or to add the max-width two places, in this case __header and __content.
I have changed it to the second approach now, which effectively does the same, but without the wrapper. Not sure which i like best.
| <ul className="opening-hours__content"> | ||
| {groupedOpeningHours.map(({ day, date, openingHourEntries }) => { | ||
| const formattedDateForDisplay = new Date(date).toLocaleDateString( | ||
| "da-DK", | ||
| { | ||
| day: "numeric", | ||
| month: "2-digit", | ||
| } | ||
| ); | ||
|
|
||
| return ( | ||
| <li key={date} className="opening-hours__row"> | ||
| <h3 className="opening-hours__individual-day">{`${day}: d. ${formattedDateForDisplay}`}</h3> | ||
| {openingHourEntries.length > 0 ? ( | ||
| <ul> |
There was a problem hiding this comment.
Consider if these lists are actually ordered (<ol> instead of <ul>).
In general I appreciate the use of list elements here.
There was a problem hiding this comment.
I think it makes sense to used OL since they atleast the weeks follow an specific sequence. 👍 Will for "categories" aswell.
|
|
||
| return ( | ||
| <li key={date} className="opening-hours__row"> | ||
| <h3 className="opening-hours__individual-day">{`${day}: d. ${formattedDateForDisplay}`}</h3> |
There was a problem hiding this comment.
Consider using a <time> element here.
| line-height: 32px; | ||
| } | ||
| } | ||
| .opening-hours__individual-opening--odd { |
There was a problem hiding this comment.
| .opening-hours__individual-opening--odd { | |
| .opening-hours__individual-opening:nth-child(odd) { |
I think this can be simplified a bit using :nth-child() like we use :first-child below.
This means we have remove the logic for applying this class also.
There was a problem hiding this comment.
Yeah this is better 👍
| .opening-hours { | ||
| .ssc-line { | ||
| height: $s-xl; | ||
| } | ||
| .ssc-line--odd { | ||
| opacity: 0.5; | ||
| } |
There was a problem hiding this comment.
👍 for making an effort by adding a skeleton.
| &:hover { | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
Consider if this is necessary as these element are <buttons> already.
There was a problem hiding this comment.
Good catch. Wasn't cleaned up after refactor where there was a "select-ish-box" too
This is the main component for displaying the openinghours for a branch. It depends on other commits for styling & sub-components. DDFFORM-417
These are subcomponents used for displaying openinghours for a branch. Depends on other commits for styling. DDFFORM-417
The example data-types are copied from the react repo. The Example data represents the grouped data after filtering in react, not the queried data. DDFFORM-417
DDFFORM-417
DDFFORM-417
Link to issue
Jira: DDFFORM-417
Description
This PR depends on two PRs.
React PR
CMS PR
This PR adds the markup and styling for the OpeningHours component, which is used to display the opening hours for a branch.
I have left chromatic unaccepted on purpose. Will accept once approved.
Screenshot of the result
Additional comments or questions
I am not sure why there are 34 UI changes.
The only changes that should be related to this pr, are the New Stories for openingHours in chromatic.
Please add comments if you have any suggestions for improvement.
This PR had a lot of refactoring after developing the React part. If you see any unused CSS or unnessecary markup mark it, although this hopefully isn't the case.