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

Website selector: searching for special strings will show html code #8467

Merged
merged 4 commits into from Aug 13, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Jul 30, 2015

fixes #7692

This worked for me. Can someone please try as well? Sometimes I can still see something like tocomplete for a second but it should be shown correctly immediately after.

@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jul 30, 2015
@tsteur tsteur added this to the 2.15.0 milestone Jul 30, 2015
updateText();
});
return {
priority: 10,
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically this is the fix. We tell the directive to execute after other directives

@mnapoli
Copy link
Contributor

mnapoli commented Jul 30, 2015

Can still see HTML code appear when typing.

Also there is this kind of encoding issue:

capture d ecran 2015-07-30 a 15 03 22

Whereas the name of the site is:

capture d ecran 2015-07-30 a 15 04 12

Other example:

capture d ecran 2015-07-30 a 15 06 19

@mnapoli
Copy link
Contributor

mnapoli commented Jul 30, 2015

Also given the obscure nature of the fix, it would be great to document it (unless another solution is applied given it doesn't solve all the problems). I don't expect anyone reading that code in a few weeks to understand why the "priority: 10".

@tsteur
Copy link
Member Author

tsteur commented Jul 30, 2015

As mentioned it can be still visible for like half a second or so but should be quickly displayed correct. Obscure nature of the fix? Do you mean the priority? Priorities are a common thing in angular and not really obscure I'd say. Can leave a comment though.

Will have a look re the problem if the search term is eg lt

@tsteur
Copy link
Member Author

tsteur commented Jul 30, 2015

Pushed another commit

@mnapoli
Copy link
Contributor

mnapoli commented Jul 30, 2015

Do you mean the priority? Priorities are a common thing in angular and not really obscure I'd say. Can leave a comment though.

priority: 10, // makes sure to render after other directives

The concept of priority is obvious, what's less obvious is why it's important to render after the other directives. That's what won't be obvious to someone landing on that code in a few weeks/months/years.

@tsteur
Copy link
Member Author

tsteur commented Jul 30, 2015

I'm not sure what to comment there. It should simply run last otherwise it could be overwritten again by another directive. Eg in my case the colors / autocomplete match was often not visible. That's why there's a priority set indicating it should run after other directives. I can link to this PR if it helps

tsteur added a commit that referenced this pull request Aug 13, 2015
Website selector: searching for special strings will show html code
@tsteur tsteur merged commit 9067f13 into master Aug 13, 2015
@tsteur tsteur deleted the 7692 branch August 13, 2015 15:26
@mattab
Copy link
Member

mattab commented Aug 13, 2015

Well done, looks fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review 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