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

[POC] UiBundle and AdminBundle #3646

Merged

Conversation

pjedrzejewski
Copy link
Member

Hello folks!

I'd like to start a discussion about the required changes to SyliusWebBundle. This PR shows the direction I want to move towards. Let me know what do you think. This is a result of plenty thoughts and discussion but I will try to summarize the most important reasons:

  • We don't have time and don't want to maintain a very custom admin interface

Yeah. We need to be realistic about how much work it is to build a nice theme. I don't think we have that many frontend devs in the community and definitely not enough time. So I want to avoid as much as possible work here. AdminLTE sounds like a perfect solution here.

  • AdminLTE is MIT licensed, based on Bootstrap and available via Bower.

MIT license - perfect. Based on Bootstrap - perfect, easy to use and everyone knows it, even for backend developers, who don't enjoy fighting with html/css too much. Available via Bower - awesome, we don't have to commit it to the repository.

  • AdminLTE has plenty of components/widgets ready - My POC is super, super simple, but the package has plenty of examples and ready to use templates - just have a look here - https://almsaeedstudio.com/preview. It will be very easily customizable!
  • Fully responsive backend out-of-the-box. Yeps, pretty much without any effort, the backend will be really responsive and mobile friendly. What we have now is far from perfect and would require great deal of work.

OK, this is about AdminLTE - I hope everyone is convinced that this is the right choice. I was considering Materialize, but tried it and it requires much more work, pretty much the same as our current custom Bootstrap backend. AdminLTE has more out-of-the-box than we could implement in next 3-6 months.


What's up with the AdminBundle and UiBundle? Goals:

  • Decouple Administration panel from the shop frontend

There are projects already which use Sylius as API and backend. We don't want to force anyone to include whole bundle with the frontend if he is not using it. Also, it allows for a nice and clean separation of Admin panel and Shop frontend. (yes, next step is ShopBundle and removing WebBundle :D)

  • Allow to reuse Sylius UI and simpler installation of standalone bundles

There is plenty of projects which use just selected bundles. Would be great if they could benefit from using Sylius UI. Also, some macros/buttons/widgets/menus/forms could be shared between bundles. Right now there is no easy way to do that, that's why I come up with the idea of SyliusUiBundle. It will contain all the AdminLTE integration stuff and common templating tools.

This will also ease the setup of individual bundles. There is no easy way now to try out some bundles. You need to copy templates or write them from scratch. With Grids (see next point) and UiBundle - that will be much much easier. In few lines, you will get a basic admin panel with nice ui in few lines of yml.

  • Grids and maintenance of templates

Not many of you know this, but we have http://docs.sylius.org/en/latest/bundles/SyliusGridBundle/index.html in works. It has been tested in a quite big project and works really well. It is not an admin generator, but follow the path that I took with ResourceBundle - having simple, configurable/resuable Crud controllers. This bundle basically adds a system of filters and grid rendering on top of SyliusResourceBundle.

The problem we have right now that there is huuuge duplication of templates in our backend. With grids, combined with SyliusUiBundle, we could remove 80% of our templates and make it much easier to customize Sylius backend. Adding new columns and filters will not require any html/css.

  • Opportunity to clean up things

Reducing amount of templates - big plus. It is also an opportunity to rework some stuff (like menu builders in this PR and translation keys).

I know that sylius_backend_tax_category_index and sylius_admin_tax_category_index are very similar, but I think that using Admin in all templates, routes etc. will make it easier for us to maintain everything admin and super simple for newcomers to understand. We got used to this "backend" thing, but Admin is more obvious.

Woah, this got pretty long. Let me hear your thoughts.

Last important note - the battle plan:

I learned the hard way to not rework everything at once. :) Idea for implementing that is to merge AdminBundle and UiBundle in the form I am proposing here (after review) and then wait for the Resource+Grids improvements to continue working on it. We would migrate resources one by one to the new backend with grids, filters and generated templates/routing. At one point, we would be left with only shop frontend in WebBundle. This would allow us to remove it in favor of new ShopBundle, which would contain the shop itself. (here there is much more work)

Sorry for any typos and rambling, it was late. ;)

@pjedrzejewski pjedrzejewski added the RFC Discussions about potential changes or new features. label Nov 28, 2015
@pjedrzejewski pjedrzejewski added this to the v1.0.0-ALPHA1 milestone Nov 28, 2015
@gperdomor
Copy link
Contributor

👍 sounds great!

@pjedrzejewski
Copy link
Member Author

I forgot to attach a screenshot!
admin-lte

@pamil
Copy link
Contributor

pamil commented Nov 28, 2015

Looks good! 👍

@@ -0,0 +1,3 @@
{
"directory": "web/assets/vendor"

Choose a reason for hiding this comment

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

I know this is just a POC but I would suggest not putting these inside the web folder. The resulting assets should be compiled into the web folder as part of a build process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree!. Bower files must be on the root and only compiled/processed files inside web. This files should be compiled/processed using asetic or grunt

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes perfect sense, thanks guys!

@peteward
Copy link

Some good ideas here and sounds very positive on the whole.

So the WebBundle would be replaced by ShopBundle (frontend) and AdminBundle (backend)? How does CoreBundle fit into this? Is Core still required to pull everything together and the other 2 bundles are purely the frontend and backend interfaces?

Would there be any domain-logic shared elements between the Shop and Admin? Like Twig extensions? How would they work? Would they be in CoreBundle? It sounds like UiBundle would be decoupled from the 'core' so wouldn't be the place for this?

I would suggest considering moving away from Assetic and consider a proper front-end driven replacement like gulp, it's much more suitable and efficient for the task and will allow for proper native implementations of CSS preprocessors such as Sass or Less. IMO it's not good for a modern app to be writing 'longhand' native CSS.

AdminLTE looks very nice, a very positive improvement. I have wanted to move away from Sylius default admin for our project but there's so many templates that would need to be changed so decided not to...

@tristanbes
Copy link
Contributor

I'm 👍 with the drop of assetic and
👍 with a gulp task to compile assets, it provides a great entry point to pre-processors, or manipulations in any kind.

great move.

@kayue
Copy link
Contributor

kayue commented Nov 29, 2015

+1 for dropping assetic

http://symfony.com/schema/dic/services/services-1.0.xsd">

<parameters>
<parameter key="sylius.menu_builder.class">Sylius\Bundle\UiBundle\Menu\MenuBuilder</parameter>
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be Sylius\Bundle\UiBundle\Menu\AbstractMenuBuilder right? Sylius\Bundle\UiBundle\Menu\MenuBuilder not exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think this is not needed at all, cause service is abstract, or am I wrong? Will check. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, you can delete the parameters block and class of the service definition because you pass a new class in the sylius.menu_builder.admin.main service definition

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@pjedrzejewski
Copy link
Member Author

Dropping Assetic - Oh yes!!! While I greatly appreciate the effort put into Assetic, it made sense only in the very beginnings of Symfony - now we have much better tools.

I was hesitant about Sylius requiring node for development. I was trying out BowerPHP and other php alternatives for node based tools, but this does not make any sense.

I want Sylius to be the best tool for both frontend and backend developers, so I am totally in favor of dropping Sylius and using all the goodies we have from npm. :)

For people not able to install node for whatever reasons or quick trying out of Sylius we will provide an already compiled package with all the vendor assets/php libraries etc.

I will come up with a super simple gulp setup but I am not an expert here by any means so I will appreciate review. :)

@peteward Both ShopBundle and AdminBundle will depend on the CoreBundle, but contain only stuff that is essential for the web interface for the store itself and backend. All API stuff and functionality not related to these web interfaces will live inside Core. Shared UI tools go to UiBundle, which will be usable outside of Sylius core.

Example use case: in one of our projects we have several microservices + Sylius. All communicating with each other via APIs. So, in full-stack Sylius application we do not need the ShopBundle at all, only AdminBundle. Another reason is that one of these microservices is about Users and Credit Cards management. For these we use only User, Addressing components. Works perfectly fine, but we need admin interface. :) Making AdminBundle generic and plug-to-play with something else than CoreBundle would be crazy amount of work, so that's why I want to use UiBundle + GridBundle for easy administration interface for applications which use only selected components.

This will also be cool for people who want to try out standalone components. Now you have to define routing, templates etc. which is definitely not the experience we want. Maintaining default templates/routing for everything is pain, so the workflow for trying out/using standalone components that I aim for is the following:

  1. Setup Symfony-Standard
  2. Plug and configure selected Sylius bundle
  3. Use grids and UiBundle for sample user interface

This is doable in about 10-15 (or even less) minutes for most of the bundles.

@pjedrzejewski
Copy link
Member Author

Also, dropping assetic and using gulp/grunt will make our life much easier when integrating #2844.

@gperdomor
Copy link
Contributor

@pjedrzejewski "Shared UI tools go to UiBundle, which will be usable outside of Sylius core"... So, this bundle must be generic to be able to use in other projects? If that is the case, we need consider which translations will be used in this bundle, because maybe i don't need tax_categories which is defined right now :D

@pjedrzejewski
Copy link
Member Author

@gperdomor Yes, you are right. :D I will open a separate RFC about all translations. I am not going to continue working on UiBundle until all ResourceBundle improvements are in (which is my main focus now) and Grids (I rebased them with master last weekend, so after SRB changes are merged, it will be quite quick to polish it, update docs etc.).

@peteward
Copy link

👍

@gperdomor
Copy link
Contributor

@pjedrzejewski 👍 Too many changes are coming to Sylius, this represent an inflexion point and so many things must be considered and polish as you said, but update the docs should be priority too :D.

Talking about Grids, any ETA for use inside sylius? maybe Sylius 0.17.0? 0.18.0?...

@gonzalovilaseca
Copy link
Contributor

👍

1 similar comment
@PWalkow
Copy link
Contributor

PWalkow commented Nov 30, 2015

👍

@adamelso
Copy link
Contributor

adamelso commented Dec 6, 2015

👍

Funny, removing Assetic is one for the first things I do when creating a new Symfony project. Nothing that can't be achieved by something like Compass or Gulp, without the ballache that comes with using Assetic (pardon the expression).

@gperdomor
Copy link
Contributor

Suggestion... Use laravel elixir to process assets. As elixir is a node package, maybe we can use node to handle other assets dependencies, like bootstrap-select for example, instead use other tools like bower.

@kayue
Copy link
Contributor

kayue commented Dec 12, 2015

@jrmyio
Copy link
Contributor

jrmyio commented Dec 12, 2015

@pjedrzejewski love what you are doing here regarding a seperate resource / grid component! Apart from FSI, you seem to be the first that really tries to apply the component/bunde seperation logic when it comes to admin related features.

@kayue I also find a rest backend with a javascript frontend very interesting but I don't see how it will work well with Twig / Symfony Forms...

@gperdomor
Copy link
Contributor

@pjedrzejewski can't install sylius/grid and sylius/grind-bundle...

The requested package sylius/grid-bundle could not be found in any version

Can you release please? i would like do some test :D

@arnolanglade
Copy link
Contributor

@pjedrzejewski 👍

@michalmarcinkowski
Copy link
Contributor

@gperdomor unfortunately the grid-bundle is not merged into master yet, so it's not available on the packagist... We will work on this bundle soon, so you can expect an update in the comming weeks.

@pjedrzejewski
Copy link
Member Author

I updated this POC with very simple Gulp tasks.

@pjedrzejewski
Copy link
Member Author

@peteward @tristanbes OK, I am switching to frontend full time anyway 💃 so I will switch to Sass. :D I went with less simply because Bootstrap 3 was using less but now I see there is official sass port dammit. Anyway switching should not be that hard.

@tristanbes I removed Bower, 2nd point of the update. :) Thanks for the suggestions. :)

@pjedrzejewski
Copy link
Member Author

Only issue here is that I will lose 1 or 2 less mixins from AdminLTE, but we can live with that I guess. :)

@pjedrzejewski
Copy link
Member Author

@peteward @tristanbes @nakashu Applied all comments, switched from LESS to Sass, font-awesome is installed via npm too.

I am not entirely happy about skin-sylius.scss because I could not longer use LESS mixins from AdminLTE to generate sidebar and navbar styles, but that's fine. AdminLTE is planning to use Sass too for next version and generally I am 👍 for Sass too.

Please give me another round of review but otherwise I'd like to get this merged tomorrow as it does not break anything and would allow me to proceed with grids much faster. :)

admin:
switch_user: true
context: user
pattern: /admin/.*

Choose a reason for hiding this comment

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

just a quick thought from a security-through-obscurity perspective, should we consider not using such a common backend access url?

Copy link
Contributor

Choose a reason for hiding this comment

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

With Sylius being open source I don't think that would be obscure enough :) IMO user experience is more important. Anyway, if someone really wants to have backend at some weird path, Symfony's Security allows to overwrite this specific configuration option (although it won't let to add new firewall options).

Choose a reason for hiding this comment

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

yeah fair enough 👍

It's just that Wordpress is /admin/ meaning every bot in the world will be looking for it - but trust in the security I guess :)

@tristanbes
Copy link
Contributor

@pjedrzejewski added a comment on a outdated diff that is not showing up: #3646 (comment)

@pjedrzejewski
Copy link
Member Author

@tristanbes @peteward I adjusted everything according to comments, I am not entirely happy with variable naming etc. but we will be iterating on this when adding more interface so nothing is set in stone here. I'd like to get this merged today and allow us to move forward. As #3819 is merged I will also add proper Behat scenarios of course.

@tristanbes
Copy link
Contributor

👍

@peteward
Copy link

definitely a good start 👍

exciting!

'sass': [
'src/Sylius/Bundle/UiBundle/Resources/private/sass/**',
],
css: [
Copy link
Contributor

Choose a reason for hiding this comment

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

@pjedrzejewski you should either use css or use 'css' but your keys should use the same convention. Right now there are mix of two styles (see js, saas, fonts, img...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally overlooked that, will fix before merge as I have to take a look at the build anyway. :)

/**
* @author Paweł Jędrzejewski <pawel@sylius.org>
*/
abstract class AbstractMenuBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

All MenuBuilders need factory and event dispatcher, we will have more of them in AdminBundle.

michalmarcinkowski added a commit that referenced this pull request Jan 21, 2016
@michalmarcinkowski michalmarcinkowski merged commit d998d0a into Sylius:master Jan 21, 2016
@michalmarcinkowski
Copy link
Contributor

Good start for better UI. Thanks Paweł! 👍

@pjedrzejewski
Copy link
Member Author

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Discussions about potential changes or new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet