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

Brief article description is showing inappropriately in popovers from Wikivoyage (at least) ZIMs scraped from new mobile HTML endpoint #1276

Open
Jaifroid opened this issue Aug 5, 2024 · 17 comments · May be fixed by #1278
Labels
bug-non-critical For bugs that it would be nice to fix rather than critical to fix good first issue user interface
Milestone

Comments

@Jaifroid
Copy link
Member

Jaifroid commented Aug 5, 2024

See screenshot to understand the issue. I believe these new brief descriptions are specific to Wikivoyage, and are only showing in ZIMs scraped from the new mobile HTML endpoint. There is nothing wrong with them, but they are not well displayed and are probably redundant in popovers. It should be easy to filter them out in popovers.js given that they have an id. Test ZIM for this is the 08-2024 Wikivoyage English maxi ZIM https://download.kiwix.org/zim/wikivoyage/wikivoyage_en_all_maxi_2024-08.zim.

image

@Jaifroid Jaifroid added good first issue user interface bug-non-critical For bugs that it would be nice to fix rather than critical to fix labels Aug 5, 2024
@Jaifroid Jaifroid added this to the v4.2 milestone Aug 5, 2024
@THEBOSS0369
Copy link

THEBOSS0369 commented Aug 25, 2024

Hey @Jaifroid ! This seems good to me i can fix it , can you tell me that how do i recreate this issue in my local website like see this image what my website is currently showing , so do i have to download that ZIM you provided in this issue and then import it in my website right? I know this question is quite basic but because of this question i can confirm two things first i'm doing correctly to recreate this issue and second is i can confirm that whether this issue is still relevant or not.

website
i have to import from brows zim library right?
website 2

@Jaifroid
Copy link
Member Author

@THEBOSS0369 Thank you for your interest in contributing. Yes, please just download the linked ZIM and then drag-and-drop into the app (or open using the file selector). Then try it out by hovering over a link in the article (probably not a link in the landing page -- try going to "Europe" and hover over a link there.

Also, to set up your dev environment, please see CONTRIBUTING.md in this Repo. When you've done that and had a look at the problem, then come back here with your suggested solution before you start coding. You can look at how text is currently selected for inclusion in a popover by having a look at the code in popovers.js.

@THEBOSS0369
Copy link

THEBOSS0369 commented Aug 27, 2024

Hey @Jaifroid ! Thanks for the step by step guide, but when i am hovering over links the popover is not showing instead the link's placeholder is showing. Can you take a look at the video . i also tried hovering in the High speed rail section but still popover didn't showed up. I have already marked the Show a popover preview of Wikipedia / Wkivoyage articles in the configure file . If there is anyother config i have to edit or any solution to show the popover it will be very helpful.

This is the video link
Link -> https://github.com/user-attachments/assets/0a3d47ba-8f25-4050-901d-89d46402f1ec

popover config
config 2

@Jaifroid
Copy link
Member Author

@THEBOSS0369 Apologies for the slow reply -- I'm away travelling currently. You were absolutely right, there was an unrelated issue blocking the display of popovers when ZIMs were being read in ServiceWorker mode without searching for a page. Hopefully that is now fixed on main, so please pull the latest commits, and you should now be able to test this issue. Note there is another probelm with that ZIM: popovers now display, but as soon as you mouse over the popover, it disappears. I believe this is to do with the changed CSS setting a different z-index, but I'm not 100% sure. ZIMs scraped from the new endpoint are currently unofficial.

@THEBOSS0369
Copy link

Hey @Jaifroid No problem sir, you can take as much time as you want. i also noticed the issue of disappearing. I have a hackathon the day after tomorrow at microsoft so i won't be able to come with a solution now but i will do it on sunday. Thanks for understanding

@Jaifroid
Copy link
Member Author

Hey @Jaifroid No problem sir, you can take as much time as you want. i also noticed the issue of disappearing. I have a hackathon the day after tomorrow at microsoft so i won't be able to come with a solution now but i will do it on sunday. Thanks for understanding

No problem. No rush at all, as I will be out of contact for much of next week.

@THEBOSS0369
Copy link

Hey @Jaifroid! i am finally done with the hackathon stuff so now i will be concentrating here! Okay so for this issue i came up with 2 approach
first is remove the brief descriptions from the popovers. Since these descriptions are redundant and not well displayed, eliminating them will make the popovers cleaner and more concise.
Second is Display Only the Relevant Content so that only relevant description will be shown and popup will look clean.
I would like to know which approach should i follow? and if there is a better way then these approaches, please let me know. Thanks

@THEBOSS0369
Copy link

Hey @Jaifroid ! If you get time, please tell me what to do.

@Jaifroid
Copy link
Member Author

Oh, apologies! I'm not sure I understand the distinction between the two approaches, but if by "remove the brief descriptions" you mean add a secondary algorithm that detects and removes them after the popover has been formatted, then no. What we want is not to select the brief descriptions in the first place, or, potentially, to remove them from selected text as soon as detected and use the final text (i.e., only the title and the lead paragraph(s)) to construct the popover. How you deselect the text of the descriptions depends on how they are referenced: do they have a clear ID or CSS class that allows us simply to deselect them? If they don't, then another method will need to be used, possibly regex-based. Thanks!

@THEBOSS0369
Copy link

THEBOSS0369 commented Sep 21, 2024

Hey @Jaifroid ! Thanks for the clarification! I'll investigate whether the brief descriptions have any identifiable characteristics like Id's Or Css classes, if they do i will modify the content extraction logic to exclude them when constructing the popovers.

If no clear identifiers are found, then I'll be using regex based approach to detect and remove the brief descriptions from the selected content while retaining only lead paragraph and title.

Will this approach would work?

Also, where should I keep you updated with the progress like here or on the slack Channel.

Thanks Again.

@Jaifroid
Copy link
Member Author

Sounds good! Best to discuss issues openly on GitHub rather than on Slack, in general. That way we keep an open history of the development process which can be useful for future devs : what worked,what didn't,etc.

@THEBOSS0369
Copy link

THEBOSS0369 commented Sep 25, 2024

Hey @Jaifroid ! Update on this issue. So, First i tried to find special classes or id, i found that all popover are being showed in a div called kiwixtooltip which is being used for every single popover, so first method didn't worked out.
Second as you suggested for regex method i implemented a very basic regex method which solved the problem here are the few ss for the popovers

Before

rail
aegen sea
mount
safari

After

rail
aegen sea
mount
safari

Please Note This method has fixed almost every popover but there are like max 3-4 popovers which are not fixed like greenland in america and causcasaus in europe. Can you tell me if there is any heading component in the codebase which is showing heading in the Bold format. If there is then it will be very easy to fix these 3-4 remaining popovers and if there is not then no problem, i will find other way.

Thanks!

@Jaifroid
Copy link
Member Author

Hi @THEBOSS0369 that's looking very good! I think the bold might be in the source HTML, but I'll check and get back to you.

@Jaifroid
Copy link
Member Author

i found that all popover are being showed in a div called kiwixtooltip which is being used for every single popover,

Just to note that "kiwixtooltip" is added by the function that constructs the popover (in popovers.js). If you look in there, you can see exactly how the popovers are constructed, and what selectors are used from the original HTML. You can even pause the programme at the point where it has access to the underlying HTML and has stored it in a variable, and take a look at it. Ideally your regex should be part of the chain of existing (commented) regexes that manipulate the content. In any case, I'll have a look to see about the bold issue.

@THEBOSS0369
Copy link

Hey @Jaifroid ! Thanks for the feedback, i will try to to look into it and if even after that the issue don't get resolved then i will create a PR maybe you can guide me more while seeing the changes in the code. Thanks ☺️

@THEBOSS0369 THEBOSS0369 linked a pull request Oct 1, 2024 that will close this issue
@THEBOSS0369
Copy link

THEBOSS0369 commented Oct 1, 2024

Hey @Jaifroid ! I have created a PR #1278 please check and also while i was testing the End-to-end (e2e) tests this error was coming i wasn't able to fix this so i tried the alternative http-server which worked fine. If there is any problem please let me know.
Thanks!

Screenshot 2024-10-01 170135

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 3, 2024

@THEBOSS0369 Sorry for being unresponsive, I've got a lot on at the moment in my day job, but I'll try to look at it properly this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-non-critical For bugs that it would be nice to fix rather than critical to fix good first issue user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants