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

Make Widgetize page translatable #19157

Merged
merged 5 commits into from May 2, 2022
Merged

Make Widgetize page translatable #19157

merged 5 commits into from May 2, 2022

Conversation

korve
Copy link
Contributor

@korve korve commented Apr 28, 2022

Description:

This PR makes the Widgetize page translatable. The related issue is #19076

Review

@sgiehl sgiehl added the Needs Review PRs that need a code review label Apr 28, 2022
@sgiehl sgiehl added this to the 4.10.0 milestone Apr 28, 2022
@sgiehl sgiehl linked an issue Apr 29, 2022 that may be closed by this pull request
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Hi @korve
Thanks for creating this pull request. Very appreciated!
I left a couple of comments, would be awesome if you could apply the changes, so this one could go into the upcoming release.

plugins/Widgetize/vue/src/ExportWidget/ExportWidget.vue Outdated Show resolved Hide resolved
plugins/Widgetize/lang/en.json Outdated Show resolved Hide resolved
plugins/Widgetize/lang/en.json Outdated Show resolved Hide resolved
plugins/Widgetize/lang/en.json Outdated Show resolved Hide resolved
plugins/Widgetize/vue/src/ExportWidget/ExportWidget.vue Outdated Show resolved Hide resolved
@korve
Copy link
Contributor Author

korve commented Apr 29, 2022

Thanks for the Review! Very insightful :) I have a question. The link to the Security Settings page is not working for me locally. The affected code is here:

<a
  rel="noreferrer noopener"
  target="_blank"
  href="${this.linkTo({ module: 'UsersManager', action: 'userSecurity' })}"
>

The generated URL is ?module=Widgetize&idSite=1&period=day&date=yesterday&params%255Bmodule%255D=UsersManager&params%255Baction%255D=userSecurity&params[module]=UsersManager&params[action]=userSecurity

Is this a problem with my host settings or is it a problem with my changes?

@sgiehl

@sgiehl
Copy link
Member

sgiehl commented Apr 29, 2022

@korve seems that was actually already broken before. There is a typo in the linkTo method. It needs to be

    linkTo(params: QueryParameters) {
      return `?${MatomoUrl.stringify({
        ...MatomoUrl.urlParsed.value,
        ...params,
      })}`;

So the three dots before params are missing. Adding them should fix the issue I guess.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Looks good now 🎉

@sgiehl
Copy link
Member

sgiehl commented May 2, 2022

I'll update the tests & vue files after merging it...

@sgiehl sgiehl merged commit bd83a44 into matomo-org:4.x-dev May 2, 2022
@sgiehl sgiehl removed the Needs Review PRs that need a code review label May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Widgetize page translatable
2 participants