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

Implement empty states aka Showcase blocks #10529

Merged
merged 19 commits into from Sep 21, 2018

Conversation

@CaptainYouz
Contributor

CaptainYouz commented Sep 20, 2018

Questions Answers
Branch? develop
Description? This PR start the work on #10434
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10434
How to test? On SEO & URL pages, you should see a Showcase block.

This change is Reviewable

@mickaelandrieu

honestly we don't need to think about a re-usable system right now, we only want to support SEO & URLS page!

I'm totally ok with something configurable in YAML like:

some_service:
     class: someClass
     arguments:
          - 'inject_the_user_locale'
          -
                fr: 'fr_link'
                es: 'spain_link'
                default: 'english_link'

We will probably do something better later, let's just make things work today 👍

@mickaelandrieu mickaelandrieu changed the title from feat(BO | Design > Pages): empty states. WIP to [WIP] Implement empty states Sep 20, 2018

@mickaelandrieu

very minor changes: go go go!

Thanks @tomas862 !

@mickaelandrieu mickaelandrieu changed the title from [WIP] Implement empty states to Implement empty states aka Showcase blocks Sep 20, 2018

@jolelievre

nice work, a few comments though :)

Show outdated Hide outdated admin-dev/themes/new-theme/scss/components/_empty-states.scss Outdated
return $link;
}
}

This comment has been minimized.

@jolelievre

jolelievre Sep 20, 2018

Contributor

Is it really necessary to have a dedicated class to manage these "link translations" ?
We could use the translator service using the english url as a key. The problem with this class is that we have to update the code each time a new language is available

@jolelievre

jolelievre Sep 20, 2018

Contributor

Is it really necessary to have a dedicated class to manage these "link translations" ?
We could use the translator service using the english url as a key. The problem with this class is that we have to update the code each time a new language is available

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

this could be refactored tomorrow: we need this pull request to be merged today

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

this could be refactored tomorrow: we need this pull request to be merged today

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 21, 2018

Contributor

or today :trollface:

@PierreRambaud

PierreRambaud Sep 21, 2018

Contributor

or today :trollface:

return $idMeta;
}
}

This comment has been minimized.

@jolelievre

jolelievre Sep 20, 2018

Contributor

could you do this without using the legacy ObjectModel, also you should create an interface for this class so that we can swap it later
once you have the Interface you could create a DoctrineMetaDataProvider which is able to fetch the data through a doctrine query builder

@jolelievre

jolelievre Sep 20, 2018

Contributor

could you do this without using the legacy ObjectModel, also you should create an interface for this class so that we can swap it later
once you have the Interface you could create a DoctrineMetaDataProvider which is able to fetch the data through a doctrine query builder

@tomas862

This comment has been minimized.

Show comment
Hide comment
@tomas862

tomas862 Sep 20, 2018

Contributor

@mickaelandrieu
If there are no more comments then it's done - PR is ready for QA 🎉

Contributor

tomas862 commented Sep 20, 2018

@mickaelandrieu
If there are no more comments then it's done - PR is ready for QA 🎉

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 20, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

I won't forget your nice review comments @jolelievre, we will work on it next week (at least, @tomas862 lol).

Contributor

mickaelandrieu commented Sep 20, 2018

I won't forget your nice review comments @jolelievre, we will work on it next week (at least, @tomas862 lol).

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 20, 2018

Contributor

Except some well known glitches when we scroll down (buttons appears under the top panel), I don't see anything wrong on this block.
Good job guys!
PS: finally added on SEO & URLs page only

Contributor

mbadrani commented Sep 20, 2018

Except some well known glitches when we scroll down (buttons appears under the top panel), I don't see anything wrong on this block.
Good job guys!
PS: finally added on SEO & URLs page only

@mbadrani mbadrani added QA ✔️ and removed waiting for QA labels Sep 20, 2018

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 20, 2018

Member

Can we have a rebase here please? Sorry about that. 🙄

Member

Quetzacoalt91 commented Sep 20, 2018

Can we have a rebase here please? Sorry about that. 🙄

* International Registered Trademark & Property of PrestaShop SA
*#}
{% trans_default_domain "Admin.Shopparameters.Feature" %}

This comment has been minimized.

@eternoendless

eternoendless Sep 20, 2018

Member

I'm not sure this will be picked up by TranslationTool

@eternoendless

eternoendless Sep 20, 2018

Member

I'm not sure this will be picked up by TranslationTool

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 20, 2018

Contributor

This is what we used in many many twig files :(

@PierreRambaud

PierreRambaud Sep 20, 2018

Contributor

This is what we used in many many twig files :(

This comment has been minimized.

@sarjon

sarjon Sep 20, 2018

Member

seriously? 😮

@sarjon

sarjon Sep 20, 2018

Member

seriously? 😮

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 21, 2018

Contributor

In all cases, this won't be fixed here, am I right?

@mickaelandrieu

mickaelandrieu Sep 21, 2018

Contributor

In all cases, this won't be fixed here, am I right?

This comment has been minimized.

@eternoendless

eternoendless Sep 21, 2018

Member

That's why I didn't ask for changes

@eternoendless

eternoendless Sep 21, 2018

Member

That's why I didn't ask for changes

@Quetzacoalt91 Quetzacoalt91 merged commit fd57aaa into PrestaShop:develop Sep 21, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 21, 2018

Member

Thank you all!

Member

Quetzacoalt91 commented Sep 21, 2018

Thank you all!

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