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

[1567] full-time part-time study change from radio buttons to checkboxes #4207

Conversation

elceebee
Copy link
Contributor

@elceebee elceebee commented May 15, 2024

Context

from the Trello ticket:
Following a review of historic design/research findings:

  1. Change from radio buttons to checkboxes
  2. Removing the ‘Full time or Part time’ field
  3. Amend wording to correlate to an either/or choice

Changes proposed in this pull request

Replace radio buttons with checkboxes when selecting study pattern, for both editing a course and creating a new course.

study.pattern.checkboxes.mov

Guidance to review

Checklist

  • Make sure all information from the Trello card is in here
  • Attach to Trello card
  • Rebased main
  • Cleaned commit history
  • Tested by running locally
  • [NA] Inform data insights team due to database changes

@elceebee elceebee marked this pull request as draft May 15, 2024 15:09
@elceebee elceebee self-assigned this May 16, 2024
@elceebee elceebee force-pushed the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch from 1683b77 to 477c1e8 Compare May 17, 2024 09:24
@elceebee elceebee changed the title replace radios with check boxes [1567] full-time part-time study change from radio buttons to checkboxes May 17, 2024
@elceebee elceebee marked this pull request as ready for review May 17, 2024 09:30
@elceebee elceebee requested a review from a team May 17, 2024 09:34
@elceebee elceebee force-pushed the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch from 477c1e8 to 8c0cc50 Compare May 17, 2024 09:40
Copy link
Contributor

@dcyoung-dev dcyoung-dev left a comment

Choose a reason for hiding this comment

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

This is way more complex than I'd have expected, there are some icky parts in this PR that are obviously from past decisions, it's scrambling my brain with the hoops you've had to jump through 🤯 good work 👌
I've left a couple of question about the specs but neither are blockers

spec/forms/publish/course_study_mode_form_spec.rb Outdated Show resolved Hide resolved
@elceebee elceebee force-pushed the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch from 8c0cc50 to 7040c02 Compare May 17, 2024 16:37
@elceebee elceebee force-pushed the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch from 598af5b to 7731046 Compare May 20, 2024 11:19
@elceebee
Copy link
Contributor Author

@elceebee elceebee force-pushed the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch from 7731046 to 829ab8b Compare May 20, 2024 12:20
Copy link
Contributor

@inulty-dfe inulty-dfe left a comment

Choose a reason for hiding this comment

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

Just a couple small concerns

config/locales/en.yml Show resolved Hide resolved
app/views/publish/courses/study_mode/new.html.erb Outdated Show resolved Hide resolved
@elceebee elceebee requested a review from inulty-dfe May 20, 2024 15:13
Copy link
Contributor

@inulty-dfe inulty-dfe 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 🚀

@elceebee elceebee force-pushed the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch from 0f79b9e to fc7f671 Compare May 21, 2024 12:06
@elceebee elceebee merged commit a44fc42 into main May 21, 2024
19 checks passed
@elceebee elceebee deleted the 1567-publish-full-time-part-time-study-change-from-radio-buttons-to-checkboxes branch May 21, 2024 13:19
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.

4 participants