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

Unify css files and move vendors assets to assets folder #7084

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Apr 18, 2021

Subject

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

Part of #7049

Requires #7081

Changelog

### Changed
- Unify css files into a single `app.css` file
- Move remaining third party assets from `src/Resources/public` to `assets` directory. Only necessary assets are exposed to the `public` dir

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@jordisala1991 jordisala1991 marked this pull request as ready for review April 18, 2021 15:09
@jordisala1991 jordisala1991 requested a review from a team April 18, 2021 15:09
@jordisala1991 jordisala1991 marked this pull request as draft April 18, 2021 15:19
@jordisala1991
Copy link
Member Author

Let's wait a little bit, have to test this one further

@VincentLanglet
Copy link
Member

Let's wait a little bit, have to test this one further

Do you use a SonataAdmin project to test this PR (and the previous) in order to check the css still works ?
I wonder if we could have some functional test for this ? With panther for example. cc @franmomu

@jordisala1991
Copy link
Member Author

I was testing the previous PR, but with this one I discovered an unrelated bug #7085 .

Also with this PR some styles broke, so probably I am missing something. Just want to make sure the styles looks the same.

@jordisala1991 jordisala1991 force-pushed the feature/unify-css-files branch 3 times, most recently from e52e535 to 723dfdb Compare April 18, 2021 19:18
@jordisala1991
Copy link
Member Author

Resolved the problems, my mistake importing files.

@jordisala1991 jordisala1991 marked this pull request as ready for review April 18, 2021 19:19
VincentLanglet
VincentLanglet previously approved these changes Apr 18, 2021
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Do you prefer to fix #7087 before a merge ?

@jordisala1991
Copy link
Member Author

There are two issues there: the first one I dont know yet how to fix, have to take a look, the second one is because I need to use the full version of select2. Anyway both issues are related to select2 js. Not related to this Pr.

Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Don't we need an upgrade note for these changes?
Regarding the changelog messages:
About the term "vendor", I think we must remove the backticks if we are using it in a colloquial way, otherwise we should specify if we are referencing a directory name or something else.
About the term "folder", my advice is to replace it with "dir" or "directory", since the folder concept is just a graphical representation for the real filesystem structure, called directory.

@jordisala1991
Copy link
Member Author

Don't we need an upgrade note for these changes?

Regarding the changelog messages:

About the term "vendor", I think we must remove the backticks if we are using it in a colloquial way, otherwise we should specify if we are referencing a directory name or something else.

About the term "folder", my advice is to replace it with "dir" or "directory", since the folder concept is just a graphical representation for the real filesystem structure, called directory.

👍 applied the changes to the changelog.

About the upgrade note, I think we wrote one on the first PR warning about the fact that we are restricting what we show on the public folder. I think still applies the same. wdyt?

@phansys
Copy link
Member

phansys commented Apr 20, 2021

About the upgrade note, I think we wrote one on the first PR warning about the fact that we are restricting what we show on the public folder. I think still applies the same. wdyt?

If the upgrade notes for these changes are already covered in a previous PR, it's fine for me.

@VincentLanglet VincentLanglet merged commit a66de70 into sonata-project:master Apr 20, 2021
@VincentLanglet
Copy link
Member

Thanks @jordisala1991

@jordisala1991 jordisala1991 deleted the feature/unify-css-files branch April 20, 2021 12:36
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