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

Bootstrap 5 views #2066

Merged
merged 7 commits into from
Dec 18, 2023
Merged

Bootstrap 5 views #2066

merged 7 commits into from
Dec 18, 2023

Conversation

kirkkwang
Copy link
Collaborator

@kirkkwang kirkkwang commented Dec 18, 2023

🧹 Upgrade views from Bootstrap 3 to 4

67f3163

This commit is a first swing of upgrading all the views from Bootstrap 3
to 4. There's still a lot of work to be done to make the views look
good but this is a good first step.

🧹 Fix CollapsableSectionPresenter override

50cad0c

The override was causing issues not passing the nav-link class into the
anchor tags. This fix specifically passes the title attribute in vs the
previous implementation of trying to pass all html options in.

🧹 Remove required translations from locales

9782c7b

This commit will remove the override for the required translations
since it changed to a badge badge-style in Bootstrap 4. This is
accounted for in Hyrax so we can fall back to that.

🐛 Fix some javascript errors

69dcb74

This commit will fix a few javascript errors, at least enough to get the
the bootstrap javascript to work correctly. We're still getting a the
almond-rails js error but that seems to exist in previous Hyku as well.

Ref:

This commit is a first swing of upgrading all the views from Bootstrap 3
to 4.  There's still a lot of work to be done to make the views look
good but this is a good first step.
@kirkkwang kirkkwang added the patch-ver for release notes label Dec 18, 2023
kirkkwang and others added 4 commits December 17, 2023 16:52
The override was causing issues not passing the nav-link class into the
anchor tags.  This fix specifically passes the title attribute in vs the
previous implementation of trying to pass all html options in.
This commit will remove the override for the `required` translations
since it changed to a badge badge-style in Bootstrap 4.  This is
accounted for in Hyrax so we can fall back to that.
This commit will fix a few javascript errors, at least enough to get the
the bootstrap javascript to work correctly.  We're still getting a the
almond-rails js error but that seems to exist in previous Hyku as well.
* 🧹Rubocop Fixes

* Attempt to fix circleci
<td><%= role.description_label %>
<td>
<%= simple_form_for :remove_role_from_group, url: admin_group_role_path(group_id: @group.to_param, role_id: role.id), method: :delete, html: { class: 'form' } do |f| %>
<%= f.submit class: 'btn btn-danger', disabled: @group.name == ::Ability.admin_group_name && role.name == 'admin', id: "remove-role-#{role.id}-from-group" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Yuck to this boolean, but I see it is carried over.

Seems like there's an ability logic that we could be writing. Because it's trivial to enable the submit button in HTML and that means the form could still be submitted to.

@laritakr
Copy link
Collaborator

I see one more spec failure than in the upgrade branch (I'm betting on a flaky spec) so I'm okaying it on a spec basis.

I didn't go through all of the code but was a bit curious about the many button changes (primary to secondary, etc). Did Hyrax change their buttons?

@kirkkwang
Copy link
Collaborator Author

I see one more spec failure than in the upgrade branch (I'm betting on a flaky spec) so I'm okaying it on a spec basis.

I didn't go through all of the code but was a bit curious about the many button changes (primary to secondary, etc). Did Hyrax change their buttons?

I'm actually surprised there's only one extra failure. Yes, button classes did change in Hyrax because of the bootstrap 3 to 4 upgrade. I saw many btn-default change to btn-secondary.

These are adjustments made after the PR has been reviewed.
Copy link
Contributor

@jeremyf jeremyf left a comment

Choose a reason for hiding this comment

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

All comments addressed!

@kirkkwang kirkkwang merged commit 5f83a9a into hyrax-5-upgrade Dec 18, 2023
2 of 3 checks passed
@kirkkwang kirkkwang deleted the bootstrap-5-views branch December 18, 2023 19:44
@kirkkwang kirkkwang mentioned this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants