Producer profile: long description and practices

openfoodfoundation / sib-discovery-components

Development and application of Startin'Blox Discovery components, initial use in Discover Regenerative www.regenerative.org.au

2 stars 0 forks source link

Producer profile: long description and practices #7

Open kirstenalarsen opened 4 months ago

kirstenalarsen commented 4 months ago

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

JbPasquier commented 4 months ago

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.

amidaOFN commented 3 months ago

Legacy markdown cleared from Airtable

mariocarabotta commented 3 months ago

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

mkllnk commented 3 months ago

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 commented 3 months ago

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

JbPasquier commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

mariocarabotta commented 2 months ago

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 commented 3 weeks ago

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