Skip to content

Conversation

atjn
Copy link
Contributor

@atjn atjn commented Jan 19, 2025

Minor improvements to product descriptions:

Make sure tags use valid code

If you left the tag color values empty, it would still send the CSS rules to the browser but with invalid colors, after which the browser would fall back to some other values. This new design is much simpler, the colors are populated by default and you cannot delete them.

Use proper formatting of product descriptions

The old interface only had a single text string for the entire product description, so people came up with hacks to make it possible to style the descriptions, for example: <h3>Månedens Øl<h3><br>September: Bonk Beer. This creates several issues, one being that it is not a good idea to use headings to make text larger, another being that this formatting looks awful when it is not supported, for example here:

image

The new interface improves on this by making three fields: name, name style and description.

image

This allows us to still use the fancy formatting, but without using headings, and in a way that also works when we dont support the fancy formatting:

image

The change is fully backwards compatible with the old database, nothing will change until we manually edit the products.

Copy link
Member

@krestenlaust krestenlaust left a comment

Choose a reason for hiding this comment

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

Doesn't this mean that TREO-personnel has more difficulty creating new events in the future, because they'd have to copy the CSS into every new product. And it makes it harder to change the style in the future, because it's defined dynamically instead of general site-wide rules

Removing the HTML also makes themes less powerful (since some products would simply override the theme, and you can't ever account for all possible product styles). And by allowing custom CSS on items, the user interface would become less streamlined (since it allows much more freedom for people not affiliated with the design of the system), I'd argue this would decrease usability/accessibility (but you're the expert in that area ;) )

(I know that you could theoretically inject javascript as the system is now and achieve this effect, but it requires much more effort as it is now, and is discouraged)

Why don't we just strip away HTML tags in the purchase output (and otherwise)?

P.s. Very nice fix for #552, I totally missed that, I should've let you review it before merging.

P.p.s. Sorry about late reply, I've recently switched to FreeBSD and my python/django setup is wip 😅

@atjn
Copy link
Contributor Author

atjn commented Jan 23, 2025

In my original design for this, I actually did not allow for custom CSS, I only allowed you to choose a "style name", which is fancy speak for setting a CSS class in the output, which would be pre-styled in our main CSS bundle. I moved away from that design because I was worried it would be too restrictive (if you want some fancy design, you need to create a PR for a new style name, which is a lot of work compared to just editing the CSS directly in the DB).

But I completely agree with all your criticism, and I would be happy to make something that is more restrictive. How restrictive do you want it? Should it still be possible to use different style names, or should there just be a single "make name bigger" checkbox, which seems to be the only thing that we really need. Or do you also want me to remove that option?

@krestenlaust
Copy link
Member

In my original design for this, I actually did not allow for custom CSS, I only allowed you to choose a "style name", which is fancy speak for setting a CSS class in the output, which would be pre-styled in our main CSS bundle. I moved away from that design because I was worried it would be too restrictive (if you want some fancy design, you need to create a PR for a new style name, which is a lot of work compared to just editing the CSS directly in the DB).

But I completely agree with all your criticism, and I would be happy to make something that is more restrictive. How restrictive do you want it? Should it still be possible to use different style names, or should there just be a single "make name bigger" checkbox, which seems to be the only thing that we really need. Or do you also want me to remove that option?

You might be right about the checkbox, but I think there's a usecase for different headers currently, I think both h2 and h1 are used, but can't remember what events that is

I actually really like the idea of having CSS classes defined, I feel this is a good compromise, this could discourage HTML injection (but still allow it), as we could just have an "event"-class, we could also have a separate style-file for these definitions, to make it more straightforward to change in the future.

@krestenlaust krestenlaust marked this pull request as draft April 22, 2025 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants