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: BodyClass depending on sections #6487

Merged
merged 3 commits into from
Nov 19, 2024
Merged

fix: BodyClass depending on sections #6487

merged 3 commits into from
Nov 19, 2024

Conversation

giuliaghisini
Copy link
Contributor

@giuliaghisini giuliaghisini commented Nov 18, 2024

When you search for a string with spaces in search site, page brokes because section body class is wrong calculated


If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.

Closes #

Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 68e1183
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/673b4678fd15250009d1af97

packages/volto/news/6487.bugfix Outdated Show resolved Hide resolved
giuliaghisini added a commit to RedTurtle/io-sanita-theme that referenced this pull request Nov 18, 2024
Copy link
Collaborator

@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.

News looks good.

This PR needs a technical review from @plone/volto-team. I'm not sure whether this is a breaking change.

@giuliaghisini can you explain the issue that this PR solves?

@ichim-david ichim-david self-requested a review November 18, 2024 17:23
@ichim-david
Copy link
Member

ichim-david commented Nov 18, 2024

@stevepiercy technically speaking it should replace all values of space with -. It's stupid that js has replace and replaceAll as such if you have more than one space only the first is replaced.
Concerning the search results I did a search and I've noticed the spaces
https://volto.demo.plone.org/search?SearchableText=News%20Page%20With%20Spaces
search-body-classes

Personally, I don't understand why we would set the searchable text as body classes.
is-adding-contenttype-?searchabletext=newspage with spaces
but maybe @sneridagh can enlighten me :)

@stevepiercy
Copy link
Collaborator

Personally, I don't understand why we would set the searchable text as body classes.

Other than it might have been a carryover from Classic UI, it is common with search engine indexing to specify the parts of the page that are indexible. Usually they exclude navigation, headers, footers, and other site-wide elements, and include <title>, <article>, or <main>. I think classes provide flexibility.

@davisagli
Copy link
Member

davisagli commented Nov 19, 2024

Personally, I don't understand why we would set the searchable text as body classes.
is-adding-contenttype-?searchabletext=newspage with spaces

That clearly looks like a bug to me. That code is trying to add a class for the Add view only, including the name of the content type being added -- but it's missing a conditional to only do it there.

it is common with search engine indexing to specify the parts of the page that are indexible. Usually they exclude navigation, headers, footers, and other site-wide elements, and include <title>,

, or

TIL, but I don't really see how that's related to this class.

@giuliaghisini
Copy link
Contributor Author

@stevepiercy technically speaking it should replace all values of space with -. It's stupid that js has replace and replaceAll as such if you have more than one space only the first is replaced. Concerning the search results I did a search and I've noticed the spaces https://volto.demo.plone.org/search?SearchableText=News%20Page%20With%20Spaces search-body-classes

Personally, I don't understand why we would set the searchable text as body classes. is-adding-contenttype-?searchabletext=newspage with spaces but maybe @sneridagh can enlighten me :)

the problem was exactly this one :-D.
I didn't understand too why searchable text is transformed in a body class.. for me it doesn't make sense..
let's see if @sneridagh knows why..

@ichim-david
Copy link
Member

@giuliaghisini @davisagli marked it as a bug and I assume that so would @sneridagh. Would you be willing since you are here to also ensure this doesn't happen in this pull request if Victor also confirms this as a bug?

@stevepiercy
Copy link
Collaborator

This PR resolves the issue with replacing multiple spaces with -, but not the issue with the funky class. @sneridagh will comment on that separately. Let's capture that in a new issue and add it to the Volto Team Meeting project.

@sneridagh
Copy link
Member

@ichim-david @giuliaghisini yeah, looks like a bug and we should address it in other PR.

@sneridagh sneridagh merged commit 3eb23a4 into main Nov 19, 2024
68 checks passed
@sneridagh sneridagh deleted the fix_bodyclass_sections branch November 19, 2024 11:04
sneridagh added a commit that referenced this pull request Nov 21, 2024
* main:
  Release 18.1.1
  Release @plone/providers 1.0.0-alpha.6
  Release @plone/components 2.2.0
  [components] Update RAC to 1.5.0, fix Disclosure import (#6498)
  Update instructions to install pipx in `RELEASING.md` (#6496)
  Refactor documentation includes to align with main documentation pattern (#6495)
  More privacy concerning youtube links and fixing link check warnings for youtube playlist links (#6494)
  fix: BodyClass depending on sections (#6487)
  Do not break toolbar if layout id is not registerd in layoutViewsNamesMapping (#6485)
  Add support for sphinxcontrib-youtube and alt tags for videos (#6486)
  Remove conflicting searchtools.js file for documentation (#6482)
  Release 18.1.0
  Release @plone/slate 18.0.1
  Add missing style Helmet serialization in the HTML component to make it work in SSR too (#6480)
  fix locales
  Dutch Translations update.  (#6476)
  Docs 6472 tidy (#6475)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants