-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
Adding @rgaudin as reviewer as he requested this improvement. On my side I have a concerns about how to translate the new hint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kelson42 That issue predates this PR and has to be fixed in the backend. |
@veloman-yunkan would this look better then? |
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). |
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.
Looks a lot better. I'd change the text to:
|
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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"
@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. |
One was png, the other was inline svg. Now both are external svg which reduce html size and enhance caching while being harmonized. What is your concern regarding svg? |
@kelson42
I dont have any particular reservation on using any, to be honest. |
@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. |
99.11% as per https://caniuse.com/svg. Pretty good i think |
This is ridiculous. Those are not large bitmap images rasterized into SVG but simple shapes which is exactly why SVG was created for. 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. |
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. |
Fixes #914