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

Parameterised feed tool tip #920

Merged
merged 2 commits into from Mar 27, 2023
Merged

Parameterised feed tool tip #920

merged 2 commits into from Mar 27, 2023

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Mar 19, 2023

Fixes #914

@kelson42
Copy link
Collaborator

Adding @rgaudin as reviewer as he requested this improvement. On my side I have a concerns about how to translate the new hint.

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

This PR generate XML which is not valid like:

<title>Filtered Entries (lang=eng&amp;category=wikipedia)</title>

I guess this is the problem because I had:
image

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Works as expected 👍 and it does look much better. Thanks.

Hopefully #900 #913 shall follow 😉

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

The parameterized message doesn't read well when more than one filtering criteria are used:

Screenshot from 2023-03-20 16-44-18

@veloman-yunkan
Copy link
Collaborator

This PR generate XML which is not valid

@kelson42 That issue predates this PR and has to be fixed in the backend.

@juuz0
Copy link
Collaborator Author

juuz0 commented Mar 20, 2023

The parameterized message doesn't read well when more than one filtering criteria are used:

@veloman-yunkan would this look better then?
Library OPDS Feed - Only for language ENG category TED matching "scope"

@veloman-yunkan
Copy link
Collaborator

The parameterized message doesn't read well when more than one filtering criteria are used:

@veloman-yunkan would this look better then? Library OPDS Feed - Only for language ENG category TED matching "scope"

Better but @rgaudin has some reservations regarding changes in capitalization. Also please make sure that the grammar is correct for any combination of filters (e.g. the current template will produce a deficient result in the presence of the search filter only).

@rgaudin
Copy link
Member

rgaudin commented Mar 20, 2023

I would not transform category names (uppercase).

I think better separators like commas between values and maybe hyphens between lang and categories might be better.

That's the kind of things that improves with time and feedback. I doubt we can have have something really satisfying using just the title attr.

Added new, better proportioned SVG files.
@juuz0
Copy link
Collaborator Author

juuz0 commented Mar 26, 2023

attempt 2 at making it look meaningful :>
image

@rgaudin
Copy link
Member

rgaudin commented Mar 27, 2023

Looks a lot better. I'd change the text to:

Library OPDS Feed – entries matching
Language: eng
Category: gutenberg, wikipedia
Query: test
  • I think the mathcing wording is more explicit than the only for one.
  • Using colons and titled parameters helps distinguish the key from the value.

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

See my previous comment

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Works for me 👍
Screenshot 2023-03-27 at 13 30 48

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

static/skin/i18n/qqq.json must be updated too

The feed logo tooltip text is now based on filters.
If no filters are set, it shows "All entries"
@kelson42
Copy link
Collaborator

@juuz0 @veloman-yunkan In this PR we have replaced pngs with svg. Does that have been discussed? Is a contol moved? At a first sight, I don’t see any benefit for that… to the contrary.

@rgaudin
Copy link
Member

rgaudin commented Mar 27, 2023

One was png, the other was inline svg. Now both are external svg which reduce html size and enhance caching while being harmonized.
Hasn't been discussed though. Was a consequence of simsie sending svg files.

What is your concern regarding svg?

@juuz0
Copy link
Collaborator Author

juuz0 commented Mar 27, 2023

@kelson42
Two things:

  1. ui language selector button was already a SVG, no replacement of filetypes here. The feed link icon was a png, this is replaced.
  2. Images in Feed Icon tooltip #914 (comment) were SVGs.

I dont have any particular reservation on using any, to be honest.

@kelson42
Copy link
Collaborator

kelson42 commented Mar 27, 2023

@rgaudin Its significantly more complex to render (energy consumption) and I worry about the support with older browsers. SVG is great if we need big images which might need to be resized (upscaling) but here we have small icons with fix rendering size AFAIK.

@juuz0
Copy link
Collaborator Author

juuz0 commented Mar 27, 2023

I worry about the support with older browsers.

99.11% as per https://caniuse.com/svg. Pretty good i think

@rgaudin
Copy link
Member

rgaudin commented Mar 27, 2023

This is ridiculous. Those are not large bitmap images rasterized into SVG but simple shapes which is exactly why SVG was created for.
It sure is mathematically more cpu intensive than simply copying bytes from memory to graphic card but this argument cannot be accepted while the whole UI is in JS.

As for browser support, it's been an obsolete question for many years.

Now SVG has one advantage over PNG in our use case that this single file is future proof in terms of resolution ; an issue we are already quite behind on: mostly at ZIM level (libzim not allowing multiple scales) but also for kiwix serve UI. It would be a small step in the right direction.

But I'd understand if we'd prefer to maintain only a single format and change everything once needed. I know we use PNG for bittorrent, magnet etc icons so consistency can also be valuable.

@kelson42
Copy link
Collaborator

Fine to me if we agree svg is appropriate, I might be a bit stuck in the past regarding browser support. I will merge, but if we agree svg is more appropriate than png, then we should benefit of such a move where we still use png files.

@kelson42 kelson42 merged commit cb20317 into main Mar 27, 2023
12 checks passed
@kelson42 kelson42 deleted the iconFeedToolTip branch March 27, 2023 20:26
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.

Feed Icon tooltip
4 participants