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.

Sign up for GitHub

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

Jump to bottom

Split BO css into multiple bundles #20584

Merged
merged 9 commits into from Sep 23, 2020
Merged

Split BO css into multiple bundles #20584

merged 9 commits into from Sep 23, 2020

Conversation

NeOMakinG
Copy link

@NeOMakinG NeOMakinG commented Aug 13, 2020 •
edited by matks

Questions Answers
Branch? develop
Description? Today, we load the whole style of the whole BO on every page, splitting the style into some bundles for every pages is avoiding to load unused styles on every pages
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
How to test? This PR modifies JS/CSS but there should be no functional change. Consequently every migrated pages should look the same as before :/ . No need to test how the page reacts on input, only how it is displayed (CSS and JS) has been changed.
Especially, look at "translate" pages, I deleted a stylesheet called "_translation_page" that looks not used anymore, couldn't find any style about it inside the BO
⚠️ Also, take some time to review product/orders pages, they are mainly aimed there

This change is Reviewable

@prestonBot prestonBot added develop Branch Bug Type: Bug labels Aug 13, 2020
@NeOMakinG NeOMakinG marked this pull request as ready for review August 18, 2020 12:34
@NeOMakinG NeOMakinG requested a review from a team as a code owner August 18, 2020 12:34
@NeOMakinG NeOMakinG changed the title [WIP] Split BO css into multiple bundles Split BO css into multiple bundles Aug 18, 2020
@prestonBot prestonBot added the Improvement Type: Improvement label Aug 18, 2020
PierreRambaud
PierreRambaud previously approved these changes Aug 18, 2020
View reviewed changes
matks
matks previously approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

@NeOMakinG Is @eternoendless OK with this change ? I know there is a debate in the frontend world about "load the full CSS once so that navigation is very smooth then" versus "only load the CSS needed for this page and consequently split bundles"

@NeOMakinG
Copy link
Author

@matks it's @eternoendless himself who asked me to work on this :)

@matks matks added the Waiting for QA Status: action required, waiting for test feedback label Aug 18, 2020
ksaandev
ksaandev previously approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@ksaandev ksaandev left a comment

Choose a reason for hiding this comment

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

Nice 😁 split bundles look better

@Progi1984 Progi1984 added this to the 1.7.8.0 milestone Sep 2, 2020
@Progi1984 Progi1984 added this to To be tested in PrestaShop 1.7.8.0 Sep 2, 2020
@Progi1984 Progi1984 removed the Bug Type: Bug label Sep 2, 2020
@ngodefroy ngodefroy self-assigned this Sep 4, 2020
@Robin-Fischer-PS
Copy link
Contributor

Hi @NeOMakinG ! Could you resolve the conflict ? Thanks :)

@Robin-Fischer-PS Robin-Fischer-PS added Waiting for author Status: action required, waiting for author feedback Waiting for dev Status: action required, waiting for tech feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 14, 2020
@Robin-Fischer-PS Robin-Fischer-PS moved this from To be tested to In progress in PrestaShop 1.7.8.0 Sep 14, 2020
@prestashop-issue-bot prestashop-issue-bot bot added the WIP Status: Work In Progress label Sep 14, 2020
@Progi1984 Progi1984 removed the Waiting for dev Status: action required, waiting for tech feedback label Sep 16, 2020
@NeOMakinG NeOMakinG requested a review from a team as a code owner September 21, 2020 09:07
@NeOMakinG NeOMakinG added Waiting for QA Status: action required, waiting for test feedback and removed WIP Status: Work In Progress Waiting for author Status: action required, waiting for author feedback labels Sep 21, 2020
@ngodefroy
Copy link

Hello,

Tested display:

Translations page:

  • Modify translation >switch french/english
  • Add language > check display on FO
  • Export language, select theme "classic" > OK
  • Copy

Orders page:

  • Add new order from BO
  • Update status
  • Check order detail
  • Deleted order
    product page:
  • Add new product
  • Edit new product
  • check preview
  • Delete product

Check migrated pages.

TEST OK

Thank you @NeOMakinG

@ngodefroy ngodefroy added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 22, 2020
@Progi1984 Progi1984 closed this Sep 22, 2020
@Progi1984 Progi1984 reopened this Sep 22, 2020
@Progi1984
Copy link
Contributor

Travis was broken 🔴

@Progi1984 Progi1984 merged commit d84803e into PrestaShop:develop Sep 23, 2020
@Progi1984
Copy link
Contributor

Thanks @NeOMakinG & @ngodefroy

@Progi1984 Progi1984 moved this from In progress to Done in PrestaShop 1.7.8.0 Sep 23, 2020
@prestashop-issue-bot prestashop-issue-bot bot added the Fixed Resolution: issue closed because fixed label Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@matks matks matks approved these changes

@ksaandev ksaandev ksaandev left review comments

Assignees

@ngodefroy ngodefroy

Labels
develop Branch Fixed Resolution: issue closed because fixed Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
No open projects
PrestaShop 1.7.8.0
  
Done
Milestone
1.7.8.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@NeOMakinG @Robin-Fischer-PS @ngodefroy @Progi1984 @PierreRambaud @matks @ksaandev @prestonBot