-
Notifications
You must be signed in to change notification settings - Fork 3
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
Scheduler Components #13
Conversation
- Adds colors package for setting shades from brand colors - Adjusts prettier config
- Adds starter assets for calendars, navigation - Adds style functions and constants - Adds date functions
- Adds styled components for general containers
- Adds basic buttons - Adds basic typography
- Adjusts export for common component, constants, and functions
- Adds date select component and styles
- Adds basic time slot select component and styles
- Adds all components to scheduler
- Removes demo components from appointments
@@ -0,0 +1,32 @@ | |||
import { h } from 'preact' | |||
|
|||
export const Calendar = () => ( |
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.
This is not quite the same calendar icon as the mocks.
It also didn't want to behave when resizing.
@@ -33,6 +34,7 @@ | |||
<script type="text/javascript"> | |||
Scheduler.init({ | |||
rootId: 'scheduler-root', | |||
// brandColor: '#0a3d00', |
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.
Uncomment to add a super gross brand color
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.
👍 mainly some comments about accessibility improvements.
*/ | ||
|
||
export const AppContainer = styled.div` | ||
height: 100vh; |
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.
This could use an overflow: auto
so if there's more content the container can scroll. Right now content will overflow outside this container.
min-height: 144px; | ||
max-height: 265px; |
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.
Are these necessary? The content should create the height needed without a min/max.
|
||
export const H3 = styled.h3` | ||
color: #000; | ||
font-size: 1rem; |
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.
👍 Verifying that the shadow dom has a root font size of 16px.
<RadioButtonList> | ||
{timeSlots.map(({ id, start, end }) => ( | ||
<RadioButtonItem key={id}> | ||
<RadioButtonInput |
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.
Using radio buttons was a bad suggestion on my part. I did a quick keyboard test and for the radio buttons:
- The radio buttons are currently missing the
name
attribute that ties a group of radio buttons together so that would need to be added, but... - The first tab into the radio group focuses on the first option but does not select it. This is why there's a "ghost" tab where focus disappears.
- Keyboard users then use the arrow keys to navigate the radio options. This moves and selects through the options (more info https://www.w3.org/wiki/RadioButton#Keyboard_interaction).
- The way the app is supposed to function (clicking an appointment time triggers the next step of confirming the appt) means that a keyboard user will always trigger the first appt in the group and never be able to move beyond it.
My recommendation is to use <button>
s (probably what you would have done if I hadn't meddled). I think this works on an accessibility level because buttons indicate triggering an action and in this case clicking an appt triggers the confirmation dialog.
setSelectedTimeSlot({ popoverOpen: false }) | ||
} | ||
|
||
const setTimeSlot = ({ id, start, provider, treatment }) => { |
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.
When triggering the confirmation modal we'll want to set focus on the modal container so keyboard/screenreader users will have their attention shifted. This is pretty straightforward by adding tabindex="-1"
to the modal container and then something like PopoverContainer.focus()
.
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.
👍 . you could also use something like focus-trap-react
here, though adding the dependency may not be worthwhile for only one thing.
popoverOpen: false, | ||
}) | ||
|
||
const cancelConfirmation = () => { |
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.
If the confirmation is cancelled, we'll want to manage focus by moving it back (see moving it to the popover below). Ideally, this would mean saving which appt was clicked and setting focus back on that. Alternately, focus could be set back to the parent appointment options container.
What Does this PR Do?
Creates basic components and styles that will primarily be used by the scheduler.
Summarized list of changes:
Screen Shots
Select Time Slot view
Confirm Popover