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

Migrate Theme & Logo #11169

Merged
merged 50 commits into from Jan 18, 2019

Conversation

Projects
None yet
7 participants
@sarjon
Copy link
Member

sarjon commented Oct 27, 2018

Questions Answers
Branch? develop
Description? Migrates Theme & Logo page.
Type? refacto
Category? CO
BC breaks? yes (legacy controller)
Deprecations? no
Fixed ticket? #10576
How to test? Access Theme & Logo page and see if it works the same as it was in legacy.

This change is Reviewable

@sarjon sarjon changed the title [WIP] Migrate Theme & Logo Migrate Theme & Logo Nov 7, 2018

Show resolved Hide resolved src/Adapter/Language/RTL/InstalledLanguageChecker.php
Show resolved Hide resolved src/Core/Addon/Theme/ThemeProvider.php
Show resolved Hide resolved src/Adapter/Shop/CommandHandler/UploadLogosHandler.php
Show resolved Hide resolved src/Adapter/Shop/Context.php Outdated
/**
* @param string $themeName
*/
public function __construct($themeName)

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

I'm still hesitating about this 🤔 whether it's over-engineered or not. An object to wrap string

This comment has been minimized.

@sarjon

sarjon Nov 8, 2018

Author Member

well, think about this way: can any string be a theme name? i think no, theme name has constraints, it cannot be empty, it must match particular format. So i think that using this kind of VOs encourages integrity, wdyt?

This comment has been minimized.

@matks

matks Nov 28, 2018

Contributor

You're right 👍

/**
* Class ProcessorFactory
*/
final class ProcessorFactory implements ProcessorFactoryInterface

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

This name is too generic, I had to look at code samples to understand what it does

$adminDir,
$themesDir,
[
$moduleDir . 'gamification',

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

Should we start a registry with those module names ? Like a PrestaShopModulesRegistry ? Wdyt ? @mickaelandrieu @PierreRambaud

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 8, 2018

Contributor

Or stored in configuration, if a user want to register his module.

/**
* @var array
*/
private $errors = [];

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

👍

@@ -8,4 +8,4 @@ _positions:

_theme:
resource: "theme.yml"
prefix: /theme
prefix: /themes

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

Isn't it a BC break ? Should'nt we keep the 2 URL paths working ?

After looking at history, this was introduced in develop AFTER the creation of 1.7.5.x so it shoud be fine, isn't it ?

This comment has been minimized.

@sarjon

sarjon Nov 8, 2018

Author Member

nope, it should be part of 1.7.6 release.

<div class="tab-pane" id="favicon-configuration" role="tabpanel" aria-labelledby="favicon-tab">
<div class="form-group">
<div class="form-control-label">
{{ ps.label_with_help(('Favicon'|trans({}, 'Admin.Design.Feature')), ('It is the small icon that appears in browser tabs, next to the web address'|trans({}, 'Admin.Design.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Nov 12, 2018

Contributor

Can you please change for It is the small icon that appears in browser tabs, next to the title? Still in Admin.Design.Help, domains are perfect. ;-)

This comment has been minimized.

@sarjon

sarjon Dec 17, 2018

Author Member

done.

</div>
<div class="col-sm">
{{ form_widget(importThemeForm.import_from_ftp) }}
<small class="form-text">{{ 'This selector lists the Zip files that you uploaded in the \'/themes\' folder.'|trans({}, 'Admin.Design.Help') }}</small>

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Nov 12, 2018

Contributor

Wording should be: This selector lists the Zip files that you uploaded in the '/themes' folder.

This comment has been minimized.

@sarjon

sarjon Dec 17, 2018

Author Member

i dont see the difference. :/

@matks matks dismissed their stale review Nov 28, 2018

Changes applied

*/
final class ProcessorFactory implements ProcessorFactoryInterface
final class StyleSheetStyleSheetProcessorFactory implements StyleSheetProcessorFactoryInterface

This comment has been minimized.

@matks

matks Nov 28, 2018

Contributor

Looks like a small mistake 😉

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 28, 2018

@sarjon Can you handle:

Then we go for QA 😄

@matks matks referenced this pull request Dec 12, 2018

Open

Migrate "Design > Theme & Logo" page #10576

1 of 6 tasks complete
@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 17, 2018

@matks all done.

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 19, 2018

@sarjon

Improvements described in specifications have not been done, can you do it ? :

  • Tabs "Configuration of home page" & "Advanced Personnalisation" should be hided when the module theme customization is disabled.
  • Logo should be resized to avoid the breaking of the page
  1. This button is missing

capture d ecran_795

  1. The button "reset to defaults" should not be displayed when debug mode is disabled

capture d ecran_796

  1. I activate multistore and multistore section and checkboxes are not displayed if i click on a shop or a group of shop

capture d ecran_797

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 19, 2018

@TristanLDD can you check if this page is ok for you ?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 19, 2018

@marionf We discussed with @TristanLDD, he is finishing a polished slightly-evolved version of this page 😉

That is why @sarjon did not include UX improvements, we're waiting for Tristan's input

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 19, 2018

@marionf regarding 3) its not available anymore in new pages, @matks do we have plan for this?

@sarjon sarjon force-pushed the sarjon:migrate/themes branch from 0ef5c16 to c214c1b Jan 16, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 16, 2019

@matks rebased, waiting for build now.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 16, 2019

@sarjon Travis is not happy with lint 😭

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 16, 2019

damn.... im sure i did run cs fixer.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jan 16, 2019

damn....

composer update && rm var/.php_cs.cache && ./vendor/bin/php-cs-fixer fix

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 17, 2019

@matks its regarding this https://prnt.sc/m7i9ct configuration, i think we didnt forget to migrate it, it was intended. i guess @mickaelandrieu could give better insight about it.

After checking with @colinegin we have to try to migrate as much as possible the multistore behaviors, including this one. So if there are other similar blocks on already-migrated pages, I'll get a list of it and we'll fix it in other PRs.

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 17, 2019

So if there are other similar blocks on already-migrated pages,

basically all configuration pages, as it was/is global behavior. i dont know how easy that would be.

@matks

matks approved these changes Jan 18, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 18, 2019

Thanks @sarjon

@matks matks merged commit e66c0e9 into PrestaShop:develop Jan 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matks matks added this to the 1.7.6.0 milestone Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment