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

Fix on asset links for the symfony layout #34394

Merged
merged 1 commit into from Nov 6, 2023

Conversation

M0rgan01
Copy link
Contributor

@M0rgan01 M0rgan01 commented Oct 26, 2023

Questions Answers
Branch? develop
Description? Fix on asset links for the symfony layout, see #34392
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? CI and UI tests are green
UI Tests legacy: https://github.com/M0rgan01/ga.tests.ui.pr/actions/runs/6665432947 / symfony: https://github.com/M0rgan01/ga.tests.ui.pr/actions/runs/6665433853
Fixed issue or discussion? Fixes ##34392
Related PRs -
Sponsor company -

@M0rgan01 M0rgan01 requested a review from a team as a code owner October 26, 2023 13:45
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Oct 26, 2023
@PrestaShop PrestaShop deleted a comment from prestonBot Oct 26, 2023
@M0rgan01 M0rgan01 added this to the 9.0.0 milestone Oct 26, 2023
nicosomb
nicosomb previously approved these changes Oct 26, 2023
tleon
tleon previously approved these changes Oct 26, 2023
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Oct 26, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

@M0rgan01 Something is still wrong my brother. It added query strings to the end of the assets, but the paths are still missing the folder prefix. 🤔 Could you check?

BEFORE
BEFORE

AFTER
AFTER


Update - it's because of the absolute path. Changing <script src="{{ asset('/js/admin.js') }}"></script> to <script src="{{ asset('js/admin.js') }}"></script> fixes the issue.

@ps-jarvis ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Oct 26, 2023
@Hlavtox Hlavtox removed the Waiting for QA Status: action required, waiting for test feedback label Oct 26, 2023
@M0rgan01 M0rgan01 dismissed stale reviews from tleon and nicosomb via 1a4f6ea October 27, 2023 09:26
@M0rgan01
Copy link
Contributor Author

M0rgan01 commented Oct 27, 2023

@Hlavtox Ok, you don't need an absolute path but rather a relative one. it should be better.

<script src="{{ asset('js/admin.js') }}"></script> does not work, because the context is under admin-dev.

@prestashop-issue-bot prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Oct 27, 2023
Hlavtox
Hlavtox previously approved these changes Oct 27, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

Works fine ;-)

boherm
boherm previously approved these changes Oct 27, 2023
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Oct 27, 2023
@florine2623 florine2623 self-assigned this Oct 30, 2023
@M0rgan01 M0rgan01 dismissed stale reviews from boherm and Hlavtox via 33bc13b October 31, 2023 09:48
@M0rgan01 M0rgan01 removed the Waiting for QA Status: action required, waiting for test feedback label Oct 31, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 31, 2023

@M0rgan01 Do you think it would it be possible to improve the paths automatically?

I think it's a bit confusing to omit the ../js in the beginning of the path, and also you need to add the front_js.

This solution requires to do some changes to template and won't solve module issues, because for example you would still get this in a module:

<script src="/admin-dev/../modules/modulename/views/js/some.bundle.js?1.7.6.9"></script>

@M0rgan01
Copy link
Contributor Author

M0rgan01 commented Oct 31, 2023

@Hlavtox

For modules, or any other folder, it is possible to add a package

framework:
  assets:
    packages:
      front_js:
        base_path: '../js'
      modules:
        base_path: '../modules'

It is also possible to navigate directly in the asset path.

For the automatic aspect of improving the paths, it is possible to create our own extension, or dynamically calculate the URL to have a complete link. But that would require more time and maintenance.

This is possible but do we want to invest time to improve paths or the impact is only visual for developers?

@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Nov 3, 2023
@florine2623
Copy link
Contributor

QA ✅
Seen with @M0rgan01, the red test has nothing to do with the PR

@florine2623 florine2623 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Nov 6, 2023
@nicosomb nicosomb merged commit 5016048 into PrestaShop:develop Nov 6, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch QA ✔️ Status: check done, code approved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants