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

Storybook integration #761

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Storybook integration #761

merged 2 commits into from
Jul 17, 2024

Conversation

M0rgan01
Copy link
Contributor

@M0rgan01 M0rgan01 commented Jul 4, 2024

Questions Answers
Description? Storybook integration - This mini-project provides an overview of the different components of the interface of the "Upgrade assistant" module in different versions of Prestashop.
Type? new feature
BC breaks? yes
Deprecations? no
Fixed ticket? -
Sponsor company -
How to test? Follow the steps in the readme to set up storybook. The components should appear as in the attached video. The autoupgrade module itself should work the same as before

BC breaks

  • *.twig files have been renamed to *.html.twig, to be consistent with CORE

To-do

  • Rename twig files
  • Make sure module still works in PS context
  • Implement most complex template to try out trans modifier, inclusion of other template and aliases
  • Include theme(s) of PS 8+
  • Make sure README is complete
  • Remove images from themes
  • Add prettier
  • Remove storybook folder on release build

To-do (later)

  • Add translations management
  • Move form generation from PHP class in templates
  • Mock ajax requests
Capture.video.du.08-07-2024.14.24.12.webm

@M0rgan01 M0rgan01 added this to the 6.0.0 milestone Jul 4, 2024
@M0rgan01 M0rgan01 marked this pull request as draft July 4, 2024 16:14
@M0rgan01 M0rgan01 force-pushed the storybook-poc branch 16 times, most recently from 54f9bd5 to 1c52c55 Compare July 8, 2024 16:20
@M0rgan01 M0rgan01 marked this pull request as ready for review July 9, 2024 13:02
@@ -0,0 +1,104 @@
# Storybook overview

This mini-project provides an overview of the different components of the interface of the "Upgrade assistant" module
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This mini-project provides an overview of the different components of the interface of the "Upgrade assistant" module
This mini-project provides an overview of the different components of the interface of the "Update assistant" module

@Quetzacoalt91 Quetzacoalt91 added the Blocked Status: The issue is blocked by another task label Jul 9, 2024
@Quetzacoalt91
Copy link
Member

Waiting for a decision to take about the themes included in the autoupgrade project or not.

@Quetzacoalt91 Quetzacoalt91 removed the Blocked Status: The issue is blocked by another task label Jul 11, 2024
@Quetzacoalt91
Copy link
Member

Unblocked: This PR will have the themes assets removed from the commits list to avoid pushing to much development stuff in the history, then another PR will be created later to add them via a NPM dependency.

@M0rgan01 M0rgan01 marked this pull request as draft July 11, 2024 13:01
@M0rgan01 M0rgan01 force-pushed the storybook-poc branch 2 times, most recently from df0a67e to 240bde7 Compare July 11, 2024 15:20
@M0rgan01 M0rgan01 marked this pull request as ready for review July 11, 2024 15:23
@Quetzacoalt91
Copy link
Member

BC breaks?

Yes, templates extensions have changed

@@ -202,7 +202,7 @@
{% endfor %}
</ul>
</td>
<td>{{ icons.nok }}</td>
<td>{{ icons.nok(psBaseUri) }}</td>
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like that we have to send the variable psBaseUri to each call, is there any other way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not very comfortable with this approach too :/ another solution is to use the _context variable, (_context.psBaseUri) but this is worse for me

Quetzacoalt91
Quetzacoalt91 previously approved these changes Jul 15, 2024
Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

Hi @M0rgan01

Thank you for your PR, I tested it and it seems to works as we expected

Tested from :
1.7.8.8 to 8.1.6
8.0.5 to 8.1.6
8.1.5 to 8.1.6
8.1.6 to 9.0.0
8.1.5 to 9.0.0
8.0.5 to 9.0.0

Because the PR seems to works as expected, It's QA ✔️

Thank you

@nicosomb nicosomb merged commit d3ff633 into PrestaShop:dev Jul 17, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants