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

AdminLTE 3 implementation #5989

Closed
wants to merge 16 commits into from
Closed

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Mar 25, 2020

Subject

You can check result here:
http://demo.wbloszyk.pl

I am targeting this branch, because it is BC-break.

Closes #5597

Changelog

### Added
- Added some `Class::newMethod` to do great stuff

### Changed

### Deprecated

### Removed

### Fixed

### Security

To do

  • Migrating to bootstrap v4
  • Migrating to adminlte v3
  • Fix tests
  • Fix Admin.js
    • tab_errors
    • x-editable
  • Check forms

Update - 07.07.2020

Current assets system have many dependencies to other Sonata bundles (like sonatacore files). It mean it is not simple to upgrade vendors and templates. It is possible to do it at once at major update but require upgrade all sonata bundles at once. To avoid it and allow upgrade sonata 3 to sonata 4 one by one bundle this thigs will be do:

  • new branch in sandbox where:
    • add encore webpack
    • add bootstrap4 and AdminLTE3
    • prepare new templates
  • after finish this branch all this change will be split to bundles

@wbloszyk wbloszyk changed the title Master AdminLTE 3 implementation Mar 25, 2020
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

composer.json Outdated
Comment on lines 28 to 30
"knplabs/knp-menu": "^2.0 || ^3.1",
"knplabs/knp-menu-bundle": "^2.3 || ^3.0",
"sonata-project/block-bundle": "^3.18.3 || ^4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why these changes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is draft. I need it for working demo for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is draft. I need it for working demo for now.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sorry for the too early review. 😅

@VincentLanglet
Copy link
Member

I think this will need an upgrade note for the user.

If he made some custom page, override, etc. He'll be happy to know which class to use instead of the previous one.

Something like,
Before / After
box -> card
box-primary -> card-primary
...

@wbloszyk
Copy link
Member Author

I think refer for adminlte and bootstrap upgrade will be enought.

@@ -0,0 +1,14 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to commit all the admin-lte files ?
Can't we declare the dependency in the composer.json or something ?
Could we use https://packagist.org/packages/almasaeed2010/adminlte ?

@VincentLanglet
Copy link
Member

I think refer for adminlte and bootstrap upgrade will be enought.

Unfortunaly the AdminLTE release notes doesnt help a lot https://github.com/ColorlibHQ/AdminLTE/releases

And I didn't found the reason for your changes in https://getbootstrap.com/docs/4.0/migration/#stable-changes

Something more precise than Update AdminLTE version from 2 to 3 could help the user.
When I took another look at your changes, and the only big change you made is actually

box box-primary

Into

card card-primary card-outline

What about adding a line in the upgrade note like

  • We're now using card card-primary card-outline instead of box box-primary ?

And if we makes some others class changes, adding a note for it.

@wbloszyk
Copy link
Member Author

You can find adminlte upgrade guide here: https://adminlte.io/docs/3.0/upgrade-guide.html

I waiting for webpack. At first i will end "Drop SonataCoreBundle" PRs. At second "add support for BlockBundle 4.0. Then i will end this PR.

BTW If adminlte and bootstrap upgrade guide is not clear enought i can add own upgrade notes.

@SonataCI
Copy link
Collaborator

SonataCI commented Apr 2, 2020

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

SonataCI commented Apr 3, 2020

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

SonataCI commented Apr 8, 2020

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@wbloszyk
Copy link
Member Author

What with #6147 ??

If x-editable will be drop then I can finish this PR.

@jordisala1991
Copy link
Member

IMHO #5461 this should be finish first, having to review 200k lines PR because we don't have assets correctly installed with npm is super hard and keep adding them makes that other PR harder and harder

@wbloszyk
Copy link
Member Author

wbloszyk commented Jun 19, 2020

I dont want psuh assets but is require for tests.

BTW I will finish wecore too.

@wbloszyk
Copy link
Member Author

#6145 (comment)

@greg0ire Can you merge 3.x branch to master in AdminBundle?

@martinbroos
Copy link

You can find adminlte upgrade guide here: https://adminlte.io/docs/3.0/upgrade-guide.html

I waiting for webpack. At first i will end "Drop SonataCoreBundle" PRs. At second "add support for BlockBundle 4.0. Then i will end this PR.

BTW If adminlte and bootstrap upgrade guide is not clear enought i can add own upgrade notes.

I'm happy to help with implementing these upgrades defined in the upgrade guide of Admin LTE3. As I can see the implementation is still incomplete. For instance admin LTE3 requires font-awesome5 which uses 'fas' instead of 'fa' when using icons.

Is it possible to ignore the webpack-encore update for now so we can get the theme implemented correctly. Others can help with this if we can get some of your code merged. If you want you can focus on 'Drop SonataCoreBundle' but my opinion is that we should not create a lot of dependencies between pull requests.

Could you fix the conflicts? Or is something else blocking this from merging ?

@wbloszyk
Copy link
Member Author

wbloszyk commented Jul 7, 2020

The problem for now is x-editable not support bootstrap 4.

@martinbroos
Copy link

Ok thanks for your response. At the moment i'm not using x-editable and are using a custom theme. So hard to say if we should hold the release for this feature. We might be able to build something like this in combination with Popper.js which is part of admin lte 3 but it's hard to get it to work in way x-editable did with all those field types.
I'm not seeing any alternative library that provides this kind of functionality.

But if the goal is using bootstrap 4 and admin lte3 my advice is to disable x-editable for now so other can help with finding a replacement or further implement this theme.

@VincentLanglet
Copy link
Member

There is an issue for this #6147
x-editable is not maintained anymore...

@wbloszyk wbloszyk added this to the 5.0 milestone Oct 21, 2020
@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Apr 19, 2021
@github-actions github-actions bot closed this Apr 26, 2021
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.

5 participants