-
-
Notifications
You must be signed in to change notification settings - Fork 756
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
Use Image from @plone/volto/components/theme/Image/Image instead of semantic-ui-react #6754
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for plone-components canceled.
|
a354789
to
df4294f
Compare
df4294f
to
c4d9310
Compare
semantic-ui-react
import LogoImage from '@plone/volto/components/theme/Logo/Logo.svg'; | ||
import { useSelector, useDispatch } from 'react-redux'; | ||
import { Link, useLocation } from 'react-router-dom'; | ||
import { getNavroot } from '@plone/volto/actions/navroot/navroot'; | ||
import { flattenToAppURL, getBaseUrl } from '@plone/volto/helpers/Url/Url'; | ||
import { hasApiExpander } from '@plone/volto/helpers/Utils/Utils'; | ||
import Image from '@plone/volto/components/theme/Image/Image'; |
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.
@wesleybl Aren't we tackling this in the main PR?
Also I found that after we upload the logo through controlpanel this fails to work as plone_site_logo
comes as site root URL and Plone doesn't know about the prefix.
https://github.com/plone/Products.CMFPlone/blob/6e2b2f55fc251a0e68b5ba65c09de6be867a9d39/Products/CMFPlone/utils.py#L508
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.
Aren't we tackling this in the main PR?
@nileshgulia1 Yes, but I'm following @sneridagh's idea of redoing PR #3592 to explain things better. So I decided to break it down into smaller, easier-to-understand PRs.
This PR practically doesn't change anything about the Volto, but it already prepares the framework for the prefix.
Also I found that after we upload the logo through controlpanel this fails to work as plone_site_logo comes as site root URL and Plone doesn't know about the prefix.
The idea here is not yet to make the prefix work.
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.
@wesleybl I will put it in this agenda in upcoming volto team meeting. Also what about the links?
@nileshgulia1 Others PRs will come if this one is approved. But for Links, the idea is to use the |
This is part of #3592, to address #4290.
That PR is to make Volto run on a subpath. For example:
http://mydomain.com/site
The idea is to use
react-router
's basename property to prefix routes. However, it doesn't add a prefix to the src of the img tag. So, to solve this, the idea is to insert the prefix in theImage
component of@plone/volto/components/theme/Image/Image
. Then all Volto images would be rendered with the Volto component.The Image component of semantic-ui-react generates an img tag. That's why we want to stop using it.
Some points of this PR:
The only thing that the
size
property of theImage
of semantic-ui-react does is add a class to the img tag. So we use theclassName
property of Volto'sImage
to simulate this behavior.The semantic-ui-react
Image
adds the "ui" and "image" classes. So we use theclassName
property of the Volto Image for this.If you agree, we can use an eslint check to prevent the semantic-ui-react component from being used in the future.