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

Producer profile: long description and practices #7

Open
kirstenalarsen opened this issue Mar 7, 2024 · 10 comments
Open

Producer profile: long description and practices #7

kirstenalarsen opened this issue Mar 7, 2024 · 10 comments
Labels
bug-s4 The bug is annoying, but doesn't prevent from using the platform. Not so many users are impacted.

Comments

@kirstenalarsen
Copy link

kirstenalarsen commented Mar 7, 2024

Looking good. Noting that there is a text formatting issue that we think is coming out of OFN . . with us to check this out
Summary:

  • OFN long description allows you to add markdown
  • That markdown was originally copied into airtable, which caused some problems
  • So Grace did some investigation and partial clean-up, but there are still some html tags that are coming through and affecting display

I am going to ask Amida to look into this. There are some relevant notes here https://github.com/orgs/openfoodfoundation/projects/10/views/2?pane=issue&itemId=47263303

Image

@JbPasquier
Copy link

Noting that there is a text formatting issue that we think is coming out of OFN . . with us to check this out

Exact, I saw some html tags there, so I managed to use them instead of stripping them.

@kirstenalarsen kirstenalarsen moved this from Testing to In Progress in Discover Regenerative Mar 13, 2024
@kirstenalarsen kirstenalarsen changed the title View producer long description and practices Producer profile: long description and practices Mar 21, 2024
@amidaOFN
Copy link

amidaOFN commented Apr 3, 2024

Legacy markdown cleared from Airtable

@mariocarabotta
Copy link

could provide a list to JB of tags to keep and clear the not allowed ones

@mkllnk
Copy link
Member

mkllnk commented Apr 3, 2024

Current list in OFN:

  ALLOWED_TAGS = ["p", "b", "strong", "em", "i", "a", "u", "br", "del", "h1", "blockquote", "pre",
                  "ul", "ol", "li", "div", "hr"].freeze                         
  ALLOWED_ATTRIBUTES = ["href", "target", "src", "alt"].freeze                  

@mariocarabotta
Copy link

I have spent some time looking at producers profiles and I would suggest to

  • remove any inline css
  • allowed tags: p, b, i, a, u, br, ul, ol, li
  • allowed attributes: href, target, src, alt
  • tempted to remove any span? those are also coming through, I don't see why we wound need any really
  • convert any h tag to p. It looks like producers are using the h tags only for styling reasons in OFN

@JbPasquier do you think this could be feasible from your end? Still not 100% sure if this suggestions are correct, just wanting to understand feasibility at this stage

@mariocarabotta mariocarabotta added the bug-s4 The bug is annoying, but doesn't prevent from using the platform. Not so many users are impacted. label Apr 3, 2024
@JbPasquier
Copy link

I can, but I'd argue that this is outside of the scope of the project.

@mkllnk doesn't your current list means that nothing else can pass through?

@mkllnk
Copy link
Member

mkllnk commented Apr 4, 2024

doesn't your current list means that nothing else can pass through?

Our editor allows only the above tags but we don't check stored HTML. An attacker could inject malicious HTML code and that would get served here. It's a security issue for us.

But yes, if you say that it's out of scope then we have to solve that within our app. But the OFN app also allows more tags than we want this component to use. For example, you can use headlines within OFN but they would look bad in the component. So ideally we would use Mario's list of allowed tags.

@mariocarabotta
Copy link

mariocarabotta commented Apr 10, 2024

@mariocarabotta to fix css after it comes through, @mkllnk to fix this later for security reasons

@mariocarabotta mariocarabotta moved this from In Progress to Ready for dev in Discover Regenerative Apr 10, 2024
@mariocarabotta mariocarabotta moved this from Ready for dev to Todo in Discover Regenerative Apr 23, 2024
@mariocarabotta
Copy link

mariocarabotta commented May 6, 2024

I have been trying to fix this, but it looks like because they are in a shadow-root it won't work

https://css-tricks.com/styling-in-the-shadow-dom-with-css-shadow-parts/
https://ionicframework.com/docs/theming/css-shadow-parts

@mariocarabotta mariocarabotta moved this from Todo to Ready for dev in Discover Regenerative May 7, 2024
@mariocarabotta mariocarabotta moved this from Ready for dev to Todo in Discover Regenerative May 7, 2024
@mariocarabotta mariocarabotta moved this from Todo to Design / Planning / Estimate in Discover Regenerative May 7, 2024
@mariocarabotta mariocarabotta moved this from Design / Planning / Estimate to Todo in Discover Regenerative Jun 19, 2024
@mariocarabotta
Copy link

waiting for this issue to be completed so that we can test this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s4 The bug is annoying, but doesn't prevent from using the platform. Not so many users are impacted.
Projects
Status: Todo
Development

No branches or pull requests

5 participants