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

[Vue] remove angularjs from the Morpheus plugin #19409

Merged
merged 29 commits into from Aug 16, 2022

Conversation

diosmosis
Copy link
Member

Description:

This PR is based off of #19391.

Changes:

  • Rewrite demo.twig to use and display Vue snippets.
  • Remove angularjs directive use from twig templates.

Review

@diosmosis diosmosis added this to the 5.0.0 milestone Jun 25, 2022
@diosmosis diosmosis marked this pull request as draft June 25, 2022 21:12
@diosmosis diosmosis added the Needs Review PRs that need a code review label Jul 11, 2022
@diosmosis diosmosis marked this pull request as ready for review July 11, 2022 02:17
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 19, 2022
@diosmosis diosmosis added the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label Jul 19, 2022
@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Jul 20, 2022
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added Stale The label used by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Jul 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added Stale The label used by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Aug 5, 2022
@sgiehl sgiehl force-pushed the vue-remove-angularjs-Morpheus branch from 77819d4 to 2a672ec Compare August 10, 2022 09:28
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.

Merged in the latest changes. Left two comments. Once they are clarified, this should be good to merge. Might be possible some screenshots needs another update...

Comment on lines 5 to 6
{% if rate is defined and rate and rate is true %}piwik-enriched-headline="{{ title|e('html_attr') }}"
{% elseif rate is defined and rate and rate %}piwik-enriched-headline="{{ rate|e('html_attr') }}"{% endif %}
{% if rate is defined and rate and rate is true %}vue-entry="CoreHome.EnrichedHeadline"
{% elseif rate is defined and rate and rate %}vue-entry="CoreHome.EnrichedHeadline"{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

Does this change make sense? Before it was either assigned title or rate to the headline. While I'm not sure if the old angular component was handling that at all, I don't think the new vue component does, or am I wrong?
Also the second condition has a duplicate and rate, which is useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

The old directive doesn't handle a piwikEnrichedHeadline attribute value so I don't think this code actually did anything before. So my reasoning is it should be fine to remove it.

rules: {
'max-len': 'off',
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this file still needed? Doesn't look like there are any lines that are too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's gone now so I'm guessing it's not.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Aug 10, 2022
@diosmosis
Copy link
Member Author

@sgiehl applied review feedback

@sgiehl sgiehl merged commit 85c79e0 into 5.x-dev Aug 16, 2022
@sgiehl sgiehl deleted the vue-remove-angularjs-Morpheus branch August 16, 2022 07:43
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 29, 2022
bx80 pushed a commit that referenced this pull request Nov 25, 2022
* remove use of angularjs from SegmentEditor plugin

* remove import added by phpstorm

* convert part of demo.twig

* make sure to export comparison service instance not just class

* built vue files

* finish converting demo.twig

* finish converting demo.twig

* remove some remaining angularjs references

* built vue files

* fixing some issues

* fix initial value for segment definition

* treat null, undefined and empty string segment value the same

* Update expected screenshots

* try to fix timing error in test

* update screenshots

* remove use of html_attr escape

* remove unneeded hmtl_attr filter use

* apply review feedback

* updates expected test files

Co-authored-by: sgiehl <stefan@matomo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not close PRs with this label won't be marked as stale by the Close Stale Issues action not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants