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 labels in anonymous mode #5650

Merged
merged 10 commits into from Sep 14, 2023
Merged

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Sep 10, 2023

fix #4305
Replaces #4649

@Alkarex Alkarex added Security 🛡️ Bug (confirmed) 🐞 issues that are reproducable labels Sep 10, 2023
@Alkarex Alkarex added this to the 1.22.0 milestone Sep 10, 2023
html += '<li class="item"><label><input class="checkboxTag" name="t_' + tag.id + '" type="checkbox"' +
(context.anonymous ? ' disabled="disabled"' : '') +
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have a look into line 1201. The event listener does not make sense in context.anonymous = true on the checkbox.

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 if (checkboxTag) { should do the job, doesn't it? In anonymous mode, this is false

Copy link
Contributor

Choose a reason for hiding this comment

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

if (ev.target.closest('.checkboxTag')) is independend of context nor disabled="disabled"

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but there is no .checkboxTag in anonymous mode, so the rest is just skipped as it should, isn't it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The class is still there

grafik

grafik

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, my mistake. Better now?

@math-GH
Copy link
Contributor

math-GH commented Sep 10, 2023

If there is no label assigned then the popup list looks a bit broken:

grafik
The title is given but nothing else

@Alkarex
Copy link
Member Author

Alkarex commented Sep 10, 2023

If there is no label assigned then the popup list looks a bit broken:

What about now?

@math-GH
Copy link
Contributor

math-GH commented Sep 11, 2023

If there is no label assigned then the popup list looks a bit broken:

What about now?

IMHO it does not make sense to list all (incl. not used) labels in the anonym. mode

grafik

I would see follwing 2 options in case of that there is no label added:

  • hide the "label" link (I guess that it would need some additional SQL statement)
  • give the information "no label set" in the pop-up

@Alkarex
Copy link
Member Author

Alkarex commented Sep 12, 2023

I have added a message when there are no labels.
Maybe it could be styled a bit better if you feel like it @math-GH

image

I would like to keep the labels also in anonymous mode, as there are more features coming related to labels (such as automatic labelling).

@math-GH
Copy link
Contributor

math-GH commented Sep 12, 2023

No labels style improved.

@math-GH
Copy link
Contributor

math-GH commented Sep 14, 2023

It looks fine now.

Does it make sense to keep the background color while hovering in this situation? There is no click action behind. As a user I would expect something there

grafik

As far as I can oversee it there is no CSS class for the anonym. mode, isn't it?

@Alkarex
Copy link
Member Author

Alkarex commented Sep 14, 2023

Does it make sense to keep the background color while hovering

I do not mind much, but I find it fine as it is, and I would rather limit the amount of code, especially when specific to a relatively niche feature

@Alkarex Alkarex merged commit bc5666c into FreshRSS:edge Sep 14, 2023
1 check passed
@Alkarex Alkarex deleted the fix-labels-anonymous branch September 14, 2023 18:23
@Frenzie
Copy link
Member

Frenzie commented Sep 14, 2023

Fwiw you might be able to do something with the new :has() selector, but wouldn't it make more sense to simply not show the checkbox instead of disabling it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug (confirmed) 🐞 issues that are reproducable Security 🛡️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] anonym. mode: my labels error
3 participants