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

Integrating Nuclia Widget #792

Draft
wants to merge 35 commits into
base: nuclia
Choose a base branch
from
Draft

Integrating Nuclia Widget #792

wants to merge 35 commits into from

Conversation

justdaksh
Copy link

CC: @stevepiercy
Here is a draft for Nuclia widget integration into training.

Some of the issues to be solved:

  • Null and Undefined results
  • Loading of widget w.r.t page and Breadcrumbs w.r.t results in Offset.

docs/_static/searchtester.js Outdated Show resolved Hide resolved
upload.py Show resolved Hide resolved
@stevepiercy
Copy link
Contributor

Here's that git command from Volto that builds only when docs change:

https://github.com/plone/volto/blob/master/netlify.toml#L5

Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

First round of review, mostly just formatting and style.

docs/_templates/search.html Outdated Show resolved Hide resolved
docs/_templates/search.html Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Contributor

I have not yet commented on design of the Nuclia widget, as I was waiting for some dust to settle. Better late than never. In general it would be good to reproduce as close as practical the existing search results listing, as @ksuess put significant thought and effort into its style and usability: https://training.plone.org/search.html?q=schema

  1. I think that the icon to the left of each listing provides nothing useful, especially because we are indexing only text. All results are text, so it feels redundant. Can it be removed?
  2. In the breadcrumb, only the first part—the training name—should be hyperlinked, and that link should be to the index of that training.
  3. The name of the page and subheadings is hyperlinked, but it should be blue instead of white.
  4. Hyperlinks should be --pst-color-primary: #579aca; ("pst" for PyData Sphinx Theme, the parent theme)
  5. I prefer sufficient whitespace over a horizontal line between each listing. Less clutter. After the above changes are made, it's possible nothing needs to be done for this item. We'll see.

If there are other things that you see, let's chat first. These are the obvious things that caught my eye.

I really like the infinite scroll loading of results as needed, and "chat with your docs" feature.

@justdaksh
Copy link
Author

cc: @stevepiercy @ebrehault

I made some changes locally and here is how the results look now:

image

image

==================================================================================
Changes:

  1. Nuclia Widget provides an option to hide thumbnails, now it forms a small icon on the left. Do we want to remove it too?
  2. Do we want only the first part of every breadcrumb, Plone Training, hyperlinked and the remaining in non-hyperlinked text as our search has every breadcrumb starting from Plone Training. Except for the last item, every other item is hyperlinked.
  3. Headings and Subheadings now are --pst-color-primary: #579aca;.
  4. Now our breadcrumbs, headings, and subheadings are of the same color.
  5. requirements.txt items have been sorted and Whitespaces have been reverted back in search.html.

Issues:

  • Null results
  • Breadcrumbs and widget loading speed.
  • Breadcrumbs Only update when you have scrolled down and results have been updated twice on scroll.

I am still working to find a better logic for the above two issues to work out.

Additional:

  1. Color change on hover on breadcrumb links from --pst-color-primary: #579aca; to --color-on-hover: #0056b3;.
  2. Widget mode is now in sync with the light/dark mode of the site.

Do we want any of these?

@stevepiercy
Copy link
Contributor

  • I don't see the changes. Please push commits to this branch so I can preview them locally and make sensible comments.
  • Toggle to light mode. You'll see. Even if you start in light mode and reload, the issue is the same. I don't know if there is a way to handle this in Nuclia's SDK. For this new issue report, because it comes up so late in the process, it would be unfair to make this a new goal. If you see something obvious, then great, go for it, but please do not spend a lot of time on it.

@netlify
Copy link

netlify bot commented Sep 21, 2023

Deploy Preview for plone-training failed. Why did it fail? →

Name Link
🔨 Latest commit 3373a37
🔍 Latest deploy log https://app.netlify.com/sites/plone-training/deploys/664c70cab959340008418baa

__pycache__/upload.cpython-310.pyc Outdated Show resolved Hide resolved
git config --global user.email github-actions@github.com
git add .
git commit -m "Nuclia Sync: Updated docs" --allow-empty
git push
Copy link
Member

Choose a reason for hiding this comment

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

So everytime there is a commit on main, we run the job, which makes a commit on main.
It looks like an infinite loop :)

Maybe the job should have a condition like if the last changes are only about our 2 generated json files, we stop.

Copy link
Author

Choose a reason for hiding this comment

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

@ebrehault I have put the check for nuclia_sync.json only. This should work, right?

upload.py Show resolved Hide resolved
@stevepiercy
Copy link
Contributor

Final items to work on:

  • Increase search item's title font size.
  • Increase space between each search result item.
  • Keep width of search results to 896px.

@stevepiercy
Copy link
Contributor

This is much easier to read. Thank you!

I found a couple issues after make livehtml and previewing locally.

  • The title link needs to have a hover state. An underline would be sufficient, and is typical. Here's what PyData theme does:
    a:hover {
      text-decoration-skip: none;
      color: var(--pst-color-link-hover);
      text-decoration-skip-ink: none;
      text-decoration-thickness: max(3px,0.1875rem,0.12em);
    }
  • In dark mode, search for schema, then hover over the "Display all results" link. It turns black. This should be readable. I'd suggest a grey that has sufficient contrast in both light and dark mode. Screenshot 2023-10-01 at 6 22 32 PM
  • When clicking a search result, it scrolls to the location of that result in the page, and highlights it with pink, which is a reserved color for inline code. Here's what the PyData theme uses:
    dt:target, span.highlighted {
      background-color: var(--pst-color-target);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants