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

Remove empty category blocks in module manager #29421

Merged
merged 2 commits into from Jan 11, 2023

Conversation

eternoendless
Copy link
Member

@eternoendless eternoendless commented Aug 23, 2022

Questions Answers
Branch? develop
Description? Whenever there is no module in a category, the block is no longer shown.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #30260
Related PRs #30810 (To merge together)
How to test? Load module manager. See the empty blocks are no longer there
Possible impacts? None?

Before:
Before

After:
After

@eternoendless eternoendless requested a review from a team as a code owner August 23, 2022 16:04
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Aug 23, 2022
ghost
ghost previously approved these changes Aug 23, 2022
Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

src/PrestaShopBundle/Resources/views/Admin/Module/Includes/grid_manage_empty.html.twig
This file has been modified recently with this hook:

{{ renderhook('displayEmptyModuleCategoryExtraMessage', {'category_name': category.name}) }}

To meet requirements from the SMB edition. @MatShir approved it

I think it looks better without this information, but it would be good to align on this topic.

@ghost
Copy link

ghost commented Aug 23, 2022

@kpodemski I don't know? there was ps_mbo for ads before? not here?

Screenshot - 2022-08-23T195418 231

@kpodemski
Copy link
Contributor

@okom3pom I don't know. I just want to avoid going back and forth with changes that were previously (not that long ago) approved :D

Hlavtox
Hlavtox previously approved these changes Aug 24, 2022
atomiix
atomiix previously approved these changes Aug 25, 2022
@MatShir
Copy link
Contributor

MatShir commented Aug 29, 2022

The issue was approved, I make sense to add content for marketplace module in the module manager #28317. UX could be enhanced ofc. It looks a good option to hide the block when nobody is hooked to it, only if we don't remove the hooks. Could you confirm @eternoendless?

@matks
Copy link
Contributor

matks commented Sep 2, 2022

Waiting for @eternoendless feedback: this PR currently disables the use of the hook mentioned by @kpodemski . So it seems we don't want to merge it like this.

@matks matks added the Waiting for author Status: action required, waiting for author feedback label Sep 2, 2022
@Hlavtox
Copy link
Contributor

Hlavtox commented Sep 2, 2022

@eternoendless @matks

{% block catalog_category_empty %}
  {% set hookData = renderhook('displayEmptyModuleCategoryExtraMessage', {'category_name': category.name}) %}
  {% if hookData is not empty %}
    <div class="modules-list module-list-empty" data-name="{{ category.refMenu }}">
      <p>{{ 'You do not have module in « %categoryName% ».' | trans({'%categoryName%': category.name}, 'Admin.Modules.Feature') }}</p>
      {{ hookData }}
    </div>
  {% endif %}
{% endblock %}

@eternoendless eternoendless dismissed stale reviews from atomiix, Hlavtox, and ghost via c2f506f September 5, 2022 16:29
@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Sep 5, 2022
@eternoendless
Copy link
Member Author

Fixed using @Hlavtox's suggestion 👍

I had to move the category name inside the block though. I also removed the wording, which not only had spelling and punctuation errors, but I also found it useless if you're adding content...

@eternoendless eternoendless removed the Waiting for author Status: action required, waiting for author feedback label Sep 5, 2022
@kpodemski kpodemski removed the Waiting for wording Status: action required, waiting for wording label Sep 13, 2022
Copy link

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

🍬

@NeOMakinG NeOMakinG added the Waiting for QA Status: action required, waiting for test feedback label Sep 15, 2022
@sLorenzini
Copy link
Contributor

hello @eternoendless,

Thank you for the PR.
To make a review in QA, we need to have an issue linked to the PR 😃 please 🙏

Thanks in advance

@sLorenzini sLorenzini added the Waiting for author Status: action required, waiting for author feedback label Oct 6, 2022
@sallemiines sallemiines self-assigned this Oct 12, 2022
@sallemiines sallemiines self-assigned this Nov 4, 2022
@kpodemski
Copy link
Contributor

@sLorenzini

I've just created an issue to unblock this PR:
#30260

Could you add proper labels to it? because it has already been accepted,

@kpodemski kpodemski removed the Waiting for author Status: action required, waiting for author feedback label Nov 10, 2022
@florine2623 florine2623 linked an issue Nov 10, 2022 that may be closed by this pull request
6 tasks
@MhiriFaten MhiriFaten self-assigned this Nov 11, 2022
Copy link

@MhiriFaten MhiriFaten left a comment

Choose a reason for hiding this comment

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

Hello @eternoendless ,

I checked this PR and I the issue is well fixed ✔️
But the automated tests are failed in BO:modules test.

image

https://github.com/MhiriFaten/testing_pr/actions/runs/3443969574/jobs/5748649304

ping @PrestaShop/qa-automation Could you please check it ?

Thank you !

@MhiriFaten MhiriFaten added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Nov 11, 2022
@MhiriFaten MhiriFaten removed their assignment Nov 11, 2022
@boubkerbribri boubkerbribri added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels Jan 11, 2023
@boubkerbribri
Copy link
Contributor

It's seems that the test should be updated, we will take a look.

@nesrineabdmouleh
Copy link
Contributor

Hello @eternoendless @MhiriFaten @boubkerbribri

Thank you for the PR and the tests !
I create this PR to fix the failed automated tests 💯

The 2 PRs should be merged together :)
QA ✔️

Thank you!

@nesrineabdmouleh nesrineabdmouleh added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jan 11, 2023
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@FabienPapet FabienPapet merged commit 303555e into PrestaShop:develop Jan 11, 2023
@matks
Copy link
Contributor

matks commented Jan 11, 2023

@FabienPapet don't forget to add the milestone ;)

@matks matks added this to the 8.1.0 milestone Jan 11, 2023
boubkerbribri added a commit that referenced this pull request Jan 11, 2023
@boubkerbribri
Copy link
Contributor

And please don't forget if there is a PR that's related (here was to fix UI tests)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove empty category blocks in module manager