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

3461 bootstrap 5 upgrade #3541

Merged
merged 21 commits into from
May 29, 2024
Merged

3461 bootstrap 5 upgrade #3541

merged 21 commits into from
May 29, 2024

Conversation

HazelGrant
Copy link
Contributor

@HazelGrant HazelGrant commented May 2, 2024

Fixes #3461

Bootstrap 5 Upgrade

Builds & tests should all pass. Each screen in this branch should look the same as its counterpart in ondemand-test.

@HazelGrant
Copy link
Contributor Author

@johrstrom @Oglopf Can you pull this down when you have a moment and check if the action buttons on the Files Browser page have outlines? I cannot for the life of me figure out why some .btn-outline-* variants seem to be not working.

@johrstrom
Copy link
Contributor

@johrstrom @Oglopf Can you pull this down when you have a moment and check if the action buttons on the Files Browser page have outlines? I cannot for the life of me figure out why some .btn-outline-* variants seem to be not working.

Sure, it's due to the $theme-colors SASS variable that we've apparently defined here which has no dark color.

// see https://getbootstrap.com/docs/4.0/getting-started/theming/#modify-map
$theme-colors: (
"primary": $blue,
"success": $green,
"danger": #d9534f
);

When we SASS compile - bootstrap will make a btn-outline-$color class for each of the colors

https://github.com/twbs/bootstrap/blob/72d3b6efc4b92e83a4abca6f5bc0cd7e6fd25931/scss/_buttons.scss#L157-L161

And again, we don't have dark color there.

I don't know why we've set that variable. I'd have to look into the git history, and even then it may not be mentioned why we did so, but it seems that after we redefine some colors, we should likely just add the defaults to that list.

https://getbootstrap.com/docs/5.0/customize/color/#theme-colors

@HazelGrant HazelGrant marked this pull request as ready for review May 21, 2024 16:18
@@ -7,14 +7,14 @@
<% end %>

<%- if Configuration.bc_saved_settings? -%>
<div class="form-group form-check">
<div class="mb3 form-check">
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be mb-3?

<div class="form-group d-flex">
<input class="form-control w-50 mr-2" name="template_name" value="<%= @template_name %>" type="text" id="batch_connect_session_template_name" aria-label="<%= t('dashboard.batch_connect_form_template_name_label') %>" readonly>
<%= f.submit t('dashboard.batch_connect_form_save_submit'), id: 'batch_connect_session_save_template_submit', class: "ml-auto btn btn-primary", formaction: batch_connect_save_settings_path(token: @app.token), disabled: @template_name ? false : true %>
<div class="mb3 d-flex">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same mb-3 typo here.

@johrstrom
Copy link
Contributor

This looks fine and works well. I just found 2 typos that we may as well fix now because they may be overlooked if we defer it.

@johrstrom johrstrom self-requested a review May 29, 2024 13:17
Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Looks good! Though we should keep #3461 open and make tickets under it when we find issues.

@johrstrom johrstrom merged commit 792cc60 into master May 29, 2024
23 checks passed
@johrstrom johrstrom deleted the 3461-bootstrap-5-upgrade branch May 29, 2024 18:51
@johrstrom johrstrom mentioned this pull request May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade to boostrap 5
3 participants