-
Notifications
You must be signed in to change notification settings - Fork 497
Add small functional updates to the Regional Workshops page #65336
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
base: staging
Are you sure you want to change the base?
Conversation
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.
Looks great! only blocker is a question on whether the regionalPartner object could by undefined
|
||
const handleSubmitZip = async () => { | ||
if (isSubmitting) { | ||
return; | ||
} |
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.
nice! this reminds me I need to guard against button mashing in the workshop form 😬
setRegionalPartnerText(rpName); | ||
const regionalPartner = | ||
jsonData.regional_workshop_data.regional_partner; | ||
if (regionalPartner.name) { |
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.
could regionalPartner
be undefined or null? how challenging would it be to refactor this component to typescript?
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.
regionalPartner
will always be defined as an object with name
and additional_info
fields. The fields themselves may be null
but the object will be defined.
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.
ah, for sure!
@@ -145,41 +168,50 @@ export default function RegionalWorkshopCatalog() { | |||
<div className={style.zipSearchInput}> | |||
<TextField | |||
id="zipSearch" | |||
name="zipCode" | |||
aria-label="zipSearch" |
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 think name
is a required prop for TextField
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.
Oh gotcha thank you! I'll add it back in!
text="Submit" | ||
color="purple" | ||
onClick={handleSubmitZip} | ||
isPending={isSubmitting} |
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.
hmm... I'm remembering that early return in your function that makes the api request. I'm wondering if maybe we should bake this into the design system, where if a button's isPending
prop is true, don't allow the onClick to fire? anyway, not relevant for this pr
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.
Yes good call, I copied this behavior too from what I saw Hannah do as well with the signup flow finish submit button. I think it'd be a great addition!
@@ -154,13 +156,16 @@ def regional_workshop_data | |||
available_workshops = workshops.select do |ws| | |||
ws.state == Pd::Workshop::STATE_NOT_STARTED && | |||
!ws.hidden && |
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.
ah, I should have noticed this before. the more of these filters that we can add into the actual database query, the better. the hidden
attribute is a boolean field on the workshops table, so it shouldn't be too tricky to move this to the where clause on line 153
in_region?(ws, partner) && | ||
has_allowed_course_for_regional_ws_page?(ws) | ||
end | ||
|
||
sorted_available_workshops = available_workshops.sort_by {|ws| ws.sessions&.first&.start} |
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 think it's technically possible to create sessions out of order, though I have to admit, I think it's extremely unlikely someone would do that 🤔
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 was also hesitant about this but saw that this line included order :start
and I did some local testing and saw that it correctly ordered sessions so I think it may be okay, but if there is a workaround that allows sessions to be out of order I'm happy to add more explicit sorting here!
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.
awesome! no that makes sense, and glad there's a sneaky rails way of doing this!
assert_equal [not_hidden_ws.id], response_workshop_ids | ||
end | ||
|
||
test 'regional_workshop_data only returns workshops that are not at capacity' do |
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 more of a product question, but would there be any value in returning "full" workshops but disable enrollment and tag it as full? and on top of that, maybe show how many spots are remaining if we don't already?
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.
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 question!
Having how many spots are left in the workshop could be a nice way to push people to sign up because of worry about missing the opportunity to sign up but if nobody is signed up could work the other way.
For showing them disabled I feel like it would make sense to show them disabled if there was a way to get on a waitlist or something but if there isn't an action I wonder how that would distract users.
@moshebaricdo what are your thoughts on this?
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.
Oo I really like these ideas @drizco! I think we should definitely add some sort of "X seats left" badge to the workshop card, it creates a sense of urgency and may help drive registrations. Also a small update to existing mocks of the workshop cards + we have the data for it.
For full workshops, I think we want to keep showing the workshop. Showing the full workshops in the catalog and having them clearly marked as “Full” feels like it adds credibility, shows that there is demand (which might also drive urgency for registration in general), and helps the catalog feel active, especially in places where there are fewer options so it doesn't get too sparse. It also sets us up for waitlist or "notify me" feature down the line. This should also be a quick mock update.
Adds small functional updates to the Regional Workshops page:
"Partner Info" dialog
Loading icon for loading workshops
Before clicking:

After clicking while loading:

Sort workshops by start date
I created 3 workshops in a shuffled order by start date to ensure that they showed up in the right order. By start date, the ids of the workshops I created went in order of 1, then 3, then 2. This is reflected in the screenshot below as expected.
Links
Jira ticket: here
Testing story
Local testing and adding unit tests.