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

Merged learning hour form partial. #5389

Merged

Conversation

Iverick
Copy link
Contributor

@Iverick Iverick commented Nov 19, 2023

What github issue is this PR for, if any?

Resolves #5372

What changed, and why?

Modified learning_hours controller and views to use single form partial. Removed update_form partial.

How will this affect user permissions?

How is this tested? (please write tests!) 💖💪

Screenshots please :)

Screenshot from 2023-11-20 00-17-57
Screenshot from 2023-11-20 00-18-06
Screenshot from 2023-11-20 00-18-23
Screenshot from 2023-11-20 00-18-29

Feelings gif (optional)

Feedback please? (optional)

We are very interested in your feedback! Please give us some :) https://forms.gle/1D5ACNgTs2u9gSdh9

@github-actions github-actions bot added ruby Pull requests that update Ruby code erb labels Nov 19, 2023
Copy link
Collaborator

@littleforest littleforest left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

app/controllers/learning_hours_controller.rb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
@Iverick Iverick force-pushed the merge_learning_hours_form_partial branch from e17b0ae to 9fb1b82 Compare November 20, 2023 03:17
Copy link
Collaborator

@littleforest littleforest left a comment

Choose a reason for hiding this comment

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

@Iverick this looks great! It doesn’t look like we have any learning_hours system specs in place though. Would you mind adding some? The spec/system/hearing_types/new_spec.rb would be a good one to copy to spec/system/learning_types/new_spec.rb and modifying, and then you could also create spec/system/learning_hours/edit_spec.rb which would be somewhat similar.

@Iverick Iverick force-pushed the merge_learning_hours_form_partial branch 3 times, most recently from 42a5f37 to c6dc60f Compare November 21, 2023 18:25
@Iverick
Copy link
Contributor Author

Iverick commented Nov 21, 2023

Updated PR to add system tests for learning hours

Copy link
Collaborator

@littleforest littleforest left a comment

Choose a reason for hiding this comment

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

Looks great! Unfortunately there are now some merge conflicts. Would you mind resolving the merge conflicts and then we can merge?

@Iverick Iverick force-pushed the merge_learning_hours_form_partial branch from c6dc60f to d20d4c1 Compare November 22, 2023 15:16
@Iverick
Copy link
Contributor Author

Iverick commented Nov 22, 2023

All right, I rebased this branch on the latest version of main to resolve conflicts and updated PR

Copy link
Collaborator

@littleforest littleforest left a comment

Choose a reason for hiding this comment

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

🎉

@littleforest littleforest merged commit 51c4427 into rubyforgood:main Nov 22, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
erb ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DRY Learning Hours Form Partial
2 participants