Skip to content

Upgrade bootswatch to version 4 #1183

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

Closed
wants to merge 7 commits into from
Closed

Conversation

bechir
Copy link
Contributor

@bechir bechir commented Dec 24, 2020

Hi,

After comparing changes before and after in #1030 I said to myself It's maybe better to create remake the job with new newly updated Symfony 5.2 (and bootswtach v5.4.3).

I know @javiereguiluz wanted to resolve the conflicts himself but you can still see the work I did here

Comparaison home

Before

home

After

home

Comparaison blog

Before

blog

After

blog

Comparaison show code

Before

show-code

After

show-code

Comparaison pagination

Before

pagination

After

pagination

Comparaison login

Before

login

After

login

Comparaison admin

Before

admin

After

admin

Comparaison edit post

Before

edit

After

edit

Comparaison show post

Before

show-post

After

show-post

Comparaison edit post (tags)

Before

post-tags

After

post-tags

Comparaison post with errors

Before

post-errors

After

post-errors

CHANGELOG

In packages.json

  • Upgraded bootswatch to version 4.5.3
  • Upgraded bootstrap to version 4.5.3
  • Removed eonasdan-bootstrap-datetimepicker dependency (This is not compatible with bootstrap 4)

In scss files

  • Removed the $icon-font-path we no longer need this
  • Moved variables import to new _variables.scss file containg our custom variables
  • Updated imports
  • Replaced hardcoded colors value with custom variables name

In javascript files

  • Replaced deprecated method submit() by trigger('submit')
  • Updated highlightjs imports

In twig files

  • Replaced well class by jumbotron with some changes in the default paddings
  • Replaced label label-* by badge badge-*
  • Replaced pull-right by float-right I d'ont kown if it's the same but we have the same result
  • Updated classes name
  • Added the active route class to active

Tests

  • Beacause the form layout changed, we need to update assertions in App\Tests\Controller\Admin\BlogControllerTest::testAdminNewPost

I didn't added datetimepicker yet because the tempusdominus/bootstrap-4 project is getting rolled back into the orginal repo, but it's still in progress.

This lib is for bootstrap 4 and it's based on the Eonasdan's Bootstrap 3 date/time picker.
But flatpickr is more powerful with zero dependencies

I didn't added it yet, I'm juste waiting for feedbacks.

Thanks for your time!

{% block form_errors -%}
{% if errors|length > 0 -%}
{% if form is not rootform %}<span class="help-block">{% else %}<div class="alert alert-danger">{% endif %}
<ul class="list-unstyled">
{%- for error in errors -%}
{# use font-awesome icon library #}
{# use font-awesome icon library # }
Copy link
Contributor

Choose a reason for hiding this comment

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

to rollback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i rolled it back and i got this
Screen Shot 2020-12-30 at 1 21 09 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the test testAdminNewDuplicatedPost method is failing because we no longer have form-group.has-error

Copy link
Contributor

Choose a reason for hiding this comment

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

i was commenting to rollback the extra space on the precise line i commented on

from

{# use font-awesome icon library #}

to
{# use font-awesome icon library # }

sorry if it was unclear :s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry.
The reason why I added that extra space is when I don't do so the code after that line will not be commented

@@ -3,6 +3,9 @@
common elements and decorates all the other templates.
See https://symfony.com/doc/current/templates.html#template-inheritance-and-layouts
#}

{% set route = app.request.get('_route') %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% set route = app.request.get('_route') %}
{% set _route = app.request.get('_route') %}

what about this?

Copy link
Contributor

@noniagriconomie noniagriconomie left a comment

Choose a reason for hiding this comment

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

nice one :)

@bechir
Copy link
Contributor Author

bechir commented Dec 30, 2020

Hi @noniagriconomie,
Thanks you for your feedbacks

I updated my code but the datetime picker initialization is still missing.
You can check flatpickr the design look good and it's about 13KB

@noniagriconomie
Copy link
Contributor

@bechir i think i am not the person who will decide if or not to replace this lib,
for me it does not matter, as the main point of this projetc is showcasing symfony etc, but lets wait main maintainers 👍

@bechir
Copy link
Contributor Author

bechir commented Dec 30, 2020

OK thanks you again.

@javiereguiluz
Copy link
Member

@bechir thanks a lot for contributing this!

The proposed changes look fantastic to me! About flatpicker, yes let's use it. I'd prefer to use the browser native datetime-local input widget, but that doesn't work in Firefox yet, so let's use flatpicker. Thanks!

<span class="fa fa-calendar" aria-hidden="true"></span>
</span>
<div class="input-group-append">
<button class="btn btn-outline-secondary">
Copy link
Member

Choose a reason for hiding this comment

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

a button without an explicit type is a very bad idea: this makes it a submit button. and this is even worse here because this is indeed inside a form...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I replaced the button by a span

Copy link
Member

Choose a reason for hiding this comment

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

using a button was the right thing to do from an accessibility point of view. But it needs an explicit type=button so that it is not a submit button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I understand. Thanks.

@bechir
Copy link
Contributor Author

bechir commented Feb 5, 2021

Thanks for your feedbacks @javiereguiluz and @stof!
I definitively need help about integrating flatpickr.

The problem is the date is not parsed correctly in admin.js

$('[data-toggle="datetimepicker"]').flatpickr({
        enableTime: true,
        dateFormat: $('#post_publishedAt').data('date-format'),
        allowInput: true,
        parseDate: (datestr, format) => {
            return moment(datestr, format, true).toDate();
        },
        formatDate: (date, format, locale) => {
            return moment(date).format(format);
        }
});

The calendar widget is shown but when I select a date nothing happen.

@atierant atierant mentioned this pull request Apr 5, 2023
@atierant
Copy link
Contributor

atierant commented Apr 6, 2023

@see 4a1d48b#diff-38752f070c2739871db0d206c9862337269e4d635f2e63e37e53c5a248c791bd for a resolution without moment.js which is deprecated

@javiereguiluz
Copy link
Member

Closing as fixed by #1412. Bechir, I'm really sorry we didn't merge this contribution ... but I'm happy that most of your work was reused as the base of #1412. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants