Upgrade Bootstrap to 5.1.3 with migrating datetimepicker to Flatpickr by mshibuya · Pull Request #3455 · railsadminteam/rails_admin

mshibuya · 2022-01-04T14:32:02Z

Closes #3083

Reason for upgrading Bootstrap to 5

Keeping our dependency up to date is necessary for our project to continue. (migrate to bootstrap 5 #3083)

Reason for replacing Bootstrap Datetimepicker with Flatpickr

Bootstrap Datetimepicker depends on Bootstrap 3. Since we're upgrading Bootstrap, it also needs to be updated.
Bootstrap Datetimepicker also depends on momentjs, which is a project stated as in maintenance mode.

Considered options

Keep using Bootstrap Datetimepicker
- Applying a minor patch may enable us to keep using this.
- But this version is no longer maintained, with the project name changed to Tempus Dominus below.
Migrate to Tempus Dominus v5
- This is a stable version, but it doesn't come with Bootstrap 5 integration (only 3 and 4).
- Also this depends on jQuery, which contradicts with our general direction of removing it.
Migrate to Tempus Dominus v6
- Having no jQuery dependency is good, but it's still in alpha.
Migrate to Flatpickr with Day.js
- Flatpickr having no jQuery dependency is good.
- Also Day.js can provide functionality equivalent to momentjs, and it can easily be integrated with Flatpickr.
- But I found working with Day.js can take a bit effort, especially with i18n things.
Migrate to Flatpickr without a date library
- This is the option I decided to use, it is simple enough while covering majority of usecases.
- But this will require users to rewrite their momentjs_format to Flatpickr's one if they use it.
Migrate to other datepicker libraries
- I took a glance at many other libraries, but many of them
  - have jQuery dependency, or dependency for other big libraries we don't want.
  - have appearances which I don't like so much (just my personal preference...!).
  - are not so actively maintained.
- If you're aware of other potential candidates please let me know!

Closes #3083

Also removed old theming functionality, since existing ones are not compatible with Bootstrap 5

coveralls · 2022-01-05T01:55:39Z

Pull Request Test Coverage Report for Build 1701174677

24 of 26 (92.31%) changed or added relevant lines in 7 files are covered.
9 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.3%) to 95.664%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/rails_admin/config/fields/types/datetime.rb	3	4	75.0%
lib/rails_admin/support/datetime.rb	5	6	83.33%

Files with Coverage Reduction	New Missed Lines	%
app/controllers/rails_admin/application_controller.rb	9	80.0%

Totals
Change from base Build 1671034350:	-0.3%
Covered Lines:	3640
Relevant Lines:	3805

💛 - Coveralls

codealchemy

Is this ready for review (and if so, would you have any additional notes / guidance for reviewers)?

Bootstrap 5 is a welcome (and significant!) change - and I started looking at the most obvious differences from the current UI - but there's quite a bit here and I'm not sure how much style / UX changes are intended, so stopped short of going further.

codealchemy · 2022-01-04T17:59:58Z

      <%= t('admin.loading') %>
    </div>
-    <nav class="navbar navbar-default navbar-fixed-top">
+    <nav class="navbar navbar-expand-xl fixed-top navbar-dark bg-primary border-bottom">


To keep the same breakpoint as before, as well as the same color / styling

Suggested change

<nav class="navbar navbar-expand-xl fixed-top navbar-dark bg-primary border-bottom">

<nav class="navbar navbar-expand-md fixed-top navbar-light bg-light border-bottom">

You're right about navbar-expand-md, but navbar-dark bg-primary are intentional. That was to make theming functionality(explained later) to stand out when using other themes, instead of just showing grayish navbar.

RocKhalil · 2022-01-06T13:11:03Z

A BIG YES FOR THIS!
I was going to work on it this coming weekend, and just saw this PR which made me happy!

@mshibuya I can review / help if you want, let me know if you need anything for this

mshibuya · 2022-01-06T13:52:18Z

@codealchemy Sorry for not giving enough context, I was lazy 🙇
I will start leaving some comments for notable changes, but it may take some time.

@RocKhalil Your review is of course welcome! Please feel free to leave questions or comments 🙏

mshibuya · 2022-01-06T14:06:04Z

      ].compact.join.html_safe
      if (edit_action = RailsAdmin::Config::Actions.find(:edit, controller: controller, abstract_model: abstract_model, object: _current_user)).try(:authorized?)
-        link_to content, rails_admin.url_for(action: edit_action.action_name, model_name: abstract_model.to_param, id: _current_user.id, controller: 'rails_admin/main')
+        link_to content, rails_admin.url_for(action: edit_action.action_name, model_name: abstract_model.to_param, id: _current_user.id, controller: 'rails_admin/main'), class: 'nav-link'


Most of these kind of changes are to keep up with the change in Bootstrap 5, like navbar needing nav-item and nav-link class for elements inside it.
https://getbootstrap.com/docs/5.1/components/navbar/

mshibuya · 2022-01-06T14:07:00Z

-      content_tag :li, class: 'dropdown', style: 'float:right' do
-        content_tag(:a, class: 'dropdown-toggle', data: {toggle: 'dropdown'}, href: '#') { t('admin.misc.bulk_menu_title').html_safe + ' ' + '<b class="caret"></b>'.html_safe } +
+      content_tag :li, class: 'nav-item dropdown dropdown-menu-end' do
+        content_tag(:a, class: 'nav-link dropdown-toggle', data: {'bs-toggle': 'dropdown'}, href: '#') { t('admin.misc.bulk_menu_title').html_safe + ' ' + '<b class="caret"></b>'.html_safe } +


Also data-toggle or data-target need to be change to data-bs-toggle or data-bs-target.

mshibuya · 2022-01-06T14:08:18Z

      return if nested_field_association?(field, nested_in)

-      @template.content_tag(:div, class: "form-group control-group #{field.type_css_class} #{field.css_class} #{'error' if field.errors.present?}", id: "#{dom_id(field)}_field") do
+      @template.content_tag(:div, class: "control-group row mb-3 #{field.type_css_class} #{field.css_class} #{'error' if field.errors.present?}", id: "#{dom_id(field)}_field") do


form-group is no longer there, so replaced with row.

mshibuya · 2022-01-06T14:18:43Z

-<li class="disabled">
-  <a href="#">
+<li class="page-item disabled">
+  <a href="#" class="page-link">


Pagination structure was also updated in Bootstrap 5.
https://getbootstrap.com/docs/5.1/components/pagination/

mshibuya · 2022-01-06T14:20:36Z

@@ -1,9 +1,5 @@
-<ul class="nav nav-pills nav-stacked">
+<ul class="col-sm-3 col-md-2 btn-toggle-nav list-unstyled bg-light">


I struggled a lot with this change because position: fixed is used here, but finally got equivalent appearance.

mshibuya · 2022-01-06T14:22:40Z

-  <div class="btn-group" data-toggle="buttons">
-    <% {'1': true, '0': false, '': nil}.each do |text, value| %>
-      <label class="<%= [field.css_classes[value], ("active" if field.form_value == value)].compact.join(' ') %> btn btn-default">
-        <%= form.radio_button field.method_name, text, field.html_attributes.reverse_merge({ checked: field.form_value == value, required: field.required}) %>


Putting an input element inside the label seemed not to work in Bootstrap 5, hence this structural change.

mshibuya · 2022-01-06T14:23:29Z

+  <div class="col-sm-2">
+    <%= form.send field.view_helper, field.method_name, field.html_attributes.reverse_merge({class: 'form-control', value: field.form_value}) %>
+  </div>
+</div>


This is for limiting width of the input element.

mshibuya · 2022-01-06T14:25:56Z

 <div class="controls col-sm-10" data-nestedmany="true">
  <div class="btn-group">
-    <a class="<%= (field.active? ? 'active' : '') %> btn btn-info toggler" data-target="<%= form.jquery_namespace(field) %> > .tab-content, <%= form.jquery_namespace(field) %> > .controls > .nav" data-toggle="button">
+    <a class="<%= (field.active? ? 'active' : 'active') %> btn btn-info toggler" data-bs-target="<%= form.jquery_namespace(field) %> .collapse" data-bs-toggle="collapse" role="button">


This active is my mistake - I forgot to remove it after testing behavior when active.

mshibuya · 2022-01-06T14:27:35Z

        </button>
      <% end %>
-      <button class="btn btn-default" data-disable-with="<%= t("admin.form.cancel") %>" formnovalidate="<%= true %>" name="_continue" type="submit">
+      <button class="btn btn-light" data-disable-with="<%= t("admin.form.cancel") %>" formnovalidate="<%= true %>" name="_continue" type="submit">


btn-default is no more.

mshibuya · 2022-01-06T14:28:51Z

@@ -1,3 +1,3 @@
-<%= rails_admin_form_for @object, url: edit_path(@abstract_model, @object.id), as: @abstract_model.param_key, html: { method: "put", multipart: true, class: "form-horizontal denser", data: { title: @page_name } } do |form| %>
+<%= rails_admin_form_for @object, url: edit_path(@abstract_model, @object.id), as: @abstract_model.param_key, html: { method: "put", multipart: true, class: "main", data: { title: @page_name } } do |form| %>


form-horizontal is no more, and added main class to be used for applying our custom styles.

mshibuya · 2022-01-06T14:30:27Z

                <% sort_direction = (sort_reverse == 'true' ? "headerSortUp" : "headerSortDown" if selected) %>
              <% end %>
-              <th class="<%= [property.sortable && "header pjax", property.sortable && sort_direction, property.sticky? && 'sticky', property.css_class, property.type_css_class].compact.join(' ') %>" data-href="<%= property.sortable && sort_location %>" rel="tooltip" title="<%= property.hint %>">
+              <th class="<%= [property.sortable && "header pjax", property.sortable && sort_direction, property.sticky? && 'sticky', property.css_class, property.type_css_class].select(&:present?).join(' ') %>" data-href="<%= property.sortable && sort_location %>" rel="tooltip" title="<%= property.hint %>">


Use of compact resulted in false showing up as CSS class, so this change.

mshibuya · 2022-01-06T14:31:18Z

-        '%z' => 'ZZ',         # Time zone as hour and minute offset from UTC (e.g. +0900)
-        '%:z' => 'Z',         # Time zone as hour and minute offset from UTC with a colon (e.g. +09:00),
+      # Ruby format options as a key and flatpickr format options as a value
+      FLATPICKR_TRANSLATIONS = {


All format options are translated into Flatpickr's ones.
https://flatpickr.js.org/formatting/

mshibuya · 2022-01-07T03:16:23Z

                .addClass(index == 0 ? "default" : "between")
                .css("display", visible ? "inline-block" : "none")
                .html(
-                  $('<input type="hidden" />')


Flatpickr's altInput functionality enabled us to remove this hidden input.

mshibuya · 2022-01-07T03:18:27Z

      is_expected.to have_content 'New Field test'
      attach_file 'Carrierwave asset', file_path('test.jpg')
      find('#modal .save-action').click
+      find('#modal .save-action').click # do it again, as a workaround for intermittent failure


mshibuya · 2022-01-07T03:19:22Z

      document.dispatchEvent(event);

      form.bind("ajax:complete", function (event) {
+        console.log(event);


Forgot to remove debug stuff 💦

mshibuya · 2022-01-07T03:19:55Z

        widget.dialog = $(
          '<div id="modal" class="modal fade">\
-            <div class="modal-dialog">\
+            <div class="modal-dialog modal-lg">\


Made the modal wider

mshibuya · 2022-01-07T03:21:58Z

      .tab-pane {
-        @include clearfix;
-        border-left: 5px solid #049cdb;
+        border-left: 5px solid map-get($theme-colors, primary);


Avoided putting color values directly, for the sake of theming functionality.

mshibuya · 2022-01-07T03:23:38Z

@@ -1 +1,3 @@
 /* RailsAdmin SASS variables */
+
+$font-size-base: 0.85rem !default;


Bootstrap5's default font size is slightly larger than Bootstrap3, so changing it smaller

mshibuya · 2022-01-07T03:24:11Z

  position: absolute;
  cursor: default;
-  z-index: 1051 !important;
+  z-index: 1056 !important;


Autocomplete popup need to come in front of modals.

mshibuya · 2022-01-07T04:54:21Z

So about theming. Since we already have webpacker-support in place we can easily integrate with Bootstrap themes like Bootswatch.

$ yarn add bootswatch

app/javascript/stylesheets/rails_admin.scss

+ @import "~bootswatch/dist/journal/variables";
  @import "~rails_admin/src/rails_admin/styles/base.scss";
+ @import "~bootswatch/dist/journal/bootswatch";

and you will get

Don't you think it's cool?

- Fix inconsistent breakpoint - Remove debug stuffs

codealchemy · 2022-01-07T21:15:36Z

-    overflow-x: auto;
-    .table {
-      margin-bottom: 0;
+    /* To silence Popper's warning: 'CSS "margin" styles cannot be used...' */


To fix the icon rendering against the span text in the nav

Suggested change

/* To silence Popper's warning: 'CSS "margin" styles cannot be used...' */

a > i {

padding: 0 0.3rem;

}

/* To silence Popper's warning: 'CSS "margin" styles cannot be used...' */

though it may be worth considering having a high-level rule for i in general (to fix the same issue elsewhere in the UI, such as removing a filter option)

Adding a whitespace between icon and text might be the better way, since we're doing so a lot elsewhere.

Sounds good (assuming that wouldn't cause issues with RTL-rendered content - which doesn't seem to be the case but mentioning it as a concern to be sure)

codealchemy · 2022-01-07T21:22:05Z

 <% visible_fields = @model_config.export.with(view: self, object: @abstract_model.model.new, controller: self.controller).visible_fields %>
-<%= form_tag export_path(params.merge(all: true)), method: 'post', class: 'form-horizontal' do %>
+<%= form_tag export_path(params.merge(all: true)), method: 'post', class: "main" do %>
  <input name="send_data" type="hidden" value="true">/</input>


(Not related to the bootstrap changes but noticed in review) this is rendering the / to the page, seems to have come from aa6ef95

…h other inputs Co-authored-by: Alex Schmitt <codealchemy@users.noreply.github.com>

- Prevent `true` showing up as a css class - form-control-sm must come with form-control: https://getbootstrap.com/docs/5.1/forms/form-control/#sizing - btn-small didn't have any effect - Fix action menu having too narrow gap between icon and text

https://github.com/railsadminteam/rails_admin/runs/4747523252?check_suite_focus=true

since all JS dependencies are ES module ready now

mshibuya · 2022-01-16T08:19:39Z

Let me merge this for now, feel free to leave comments here and I'll address them!

…hey need https://github.com/railsadminteam/rails_admin/wiki/Translations#flatpickr Refs. #3455

During the bootstrap 5 update in 6878670 (railsadminteam#3455) this class was changed from `content` to `container`. The `container` class, unlike `content`, has a max width of 1320px. Restore prior behavior of using the full width by using `container-fluid` to avoid wasted whitespace.

During the bootstrap 5 update in 6878670 (#3455) this class was changed from `content` to `container`. The `container` class, unlike `content`, has a max width of 1320px. Restore prior behavior of using the full width by using `container-fluid` to avoid wasted whitespace.

mshibuya added 3 commits January 4, 2022 22:24

Upgrade Bootstrap to 5.1.3

a4e513d

Closes #3083

Upgrade vendored Bootstrap as well

15d8404

Also removed old theming functionality, since existing ones are not compatible with Bootstrap 5

Migrate datetimepicker widget to flatpickr

dbd01d6

codealchemy reviewed Jan 5, 2022

View reviewed changes

mshibuya commented Jan 6, 2022

View reviewed changes

mshibuya commented Jan 7, 2022

View reviewed changes

Reflect review and minor fix

242ada3

- Fix inconsistent breakpoint - Remove debug stuffs

codealchemy reviewed Jan 7, 2022

View reviewed changes

Apply form-control-sm to make datepicker field have the same size wit…

16e72b5

…h other inputs Co-authored-by: Alex Schmitt <codealchemy@users.noreply.github.com>

mshibuya added 2 commits January 8, 2022 19:20

Reflect review suggetions

9776139

- Prevent `true` showing up as a css class - form-control-sm must come with form-control: https://getbootstrap.com/docs/5.1/forms/form-control/#sizing - btn-small didn't have any effect - Fix action menu having too narrow gap between icon and text

Fix invalid closing tag </input>

c2b155e

mshibuya force-pushed the bootstrap5 branch from 09fdf65 to ff88f40 Compare January 8, 2022 10:20

Update styles in Export and History action

7911dbd

mshibuya force-pushed the bootstrap5 branch from ff88f40 to 7911dbd Compare January 9, 2022 03:30

Fix spec issue, with removing the workaround

f75c853

https://github.com/railsadminteam/rails_admin/runs/4747523252?check_suite_focus=true

mshibuya mentioned this pull request Jan 10, 2022

Fix issues with parsing localized date/time flatpickr/flatpickr#2600

Merged

mshibuya added 2 commits January 15, 2022 15:36

Add additional datetime format used somewhere in rails-i18n

4ea777a

ProvidePlugin is no longer necessary

e83801c

since all JS dependencies are ES module ready now

mshibuya changed the title ~~Upgrade Bootstrap to 5.1.3~~ Upgrade Bootstrap to 5.1.3 with migrating datetimepicker to Flatpickr Jan 15, 2022

Merge branch 'master' into bootstrap5

49fec4e

mshibuya merged commit 6878670 into master Jan 16, 2022

mshibuya deleted the bootstrap5 branch January 16, 2022 08:22

mshibuya added a commit that referenced this pull request Jan 16, 2022

Stop dynamically importing Flatpickr locales, let users choose what t…

186d080

…hey need https://github.com/railsadminteam/rails_admin/wiki/Translations#flatpickr Refs. #3455

wartron mentioned this pull request Feb 11, 2022

3.0.0.rc causes issues with webpacker #3467

Closed

codealchemy mentioned this pull request Mar 9, 2022

Bootstrap 5 followup: fix show views #3491

Merged

q3aiml mentioned this pull request Mar 10, 2022

restore full width for content #3493

Merged

	<nav class="navbar navbar-expand-xl fixed-top navbar-dark bg-primary border-bottom">
	<nav class="navbar navbar-expand-md fixed-top navbar-light bg-light border-bottom">

		@@ -1,9 +1,5 @@
		<ul class="nav nav-pills nav-stacked">
		<ul class="col-sm-3 col-md-2 btn-toggle-nav list-unstyled bg-light">

		@@ -1,3 +1,3 @@
		<%= rails_admin_form_for @object, url: edit_path(@abstract_model, @object.id), as: @abstract_model.param_key, html: { method: "put", multipart: true, class: "form-horizontal denser", data: { title: @page_name } } do \|form\| %>
		<%= rails_admin_form_for @object, url: edit_path(@abstract_model, @object.id), as: @abstract_model.param_key, html: { method: "put", multipart: true, class: "main", data: { title: @page_name } } do \|form\| %>

		@@ -1 +1,3 @@
		/* RailsAdmin SASS variables */

		$font-size-base: 0.85rem !default;

Conversation

mshibuya commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for upgrading Bootstrap to 5

Reason for replacing Bootstrap Datetimepicker with Flatpickr

Considered options

Uh oh!

coveralls commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1701174677

💛 - Coveralls

Uh oh!

codealchemy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RocKhalil commented Jan 6, 2022

Uh oh!

mshibuya commented Jan 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshibuya Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshibuya commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshibuya commented Jan 4, 2022 •

edited

Loading

coveralls commented Jan 5, 2022 •

edited

Loading

mshibuya Jan 7, 2022 •

edited

Loading

mshibuya commented Jan 7, 2022 •

edited

Loading