django-cms / djangocms-frontend

django CMS frontend is a plugin bundle for django CMS providing several components for the frontend, currently implemented with the popular Bootstrap 5 framework.
Other
43 stars 20 forks source link

Figures do not properly render #63

Closed greyhare closed 1 year ago

greyhare commented 1 year ago

Describe the bug The figure plugin doesn't render HTML resembling the Bootstrap 5 spec.

To Reproduce Steps to reproduce the behavior:

  1. Create a Figure plugin
  2. Set a caption and a non-default alignment, such as centered. Also add a drop shadow.
  3. Add an image to it.
  4. See the caption left-justified and no drop shadow.
  5. Look at HTML source.
  6. Note no figure or shadow classes on the figure, no figure-img class on the image, and no text-center class on the caption.

Expected behavior A figure with working extra styles and a caption whose alignment you can set.

Desktop (please complete the following information):

Additional context

  1. In contrib/content/frameworks.bootstrap5.py the FigureRenderMixin.render method is adding the aliugnment to the figure tag's classes when it should not (lines 16 and 17).
  2. In contrib/content/templates/djangocms_frontend/bootstrap5/figure.html, get_attributes is misspelled (line 3).
  3. In contrib/content/templates/djangocms_frontend/bootstrap5/figure.html, the figcaption class should be "figure-caption{% if instance.figure_alignment %} text-{{ instance.figure_alignment }}{% endif %}" (line 7).
  4. I have no idea how to add the figure-img class to a contained img from an Image/Picture plugin. (Also, the Image/Picture plugin has a caption field itself which seems to not be used anywhere...?)
greyhare commented 1 year ago

Also, why are arbitrary contained plugins allowed (i.e. anything other than a single image)? Bootstrap doesn't mention that as an option.

fsbraun commented 1 year ago

Hello @greyhare ! Thanks for pointing this out! I'll looking at it asap. As to why not restricting the child plugins: Some installations have their own Image plugin which is why I refrain from restricting flexibility more than technically necessary.

fsbraun commented 1 year ago

Thinking of it: Should the caption not be html to allow for bold, italic or whatever? If I change this to a HTML field you'd also get a wrapping <p>...</p>. This is not in the bootstrap documentation. What do you think?

greyhare commented 1 year ago

<figcaption> can contain flow content, such as paragraphs, though the current stylesheet would likely stuff in an ugly margin.

There is already a need for captions in Blockquote to work better. The example in the Bootstrap docs cannot be implemented in djangocms-frontend because frontend forces the whole caption to be in the <cite> tag.

fsbraun commented 1 year ago

The cite tag is a bug there, too. It needs to be removed.

If I turn the caption into an HTMLField text will be surrounded by a paragraph <p> probably creating the margin that one doesn't want. IHO this p tag can be safely removed as long as there is only one paragraph.

greyhare commented 1 year ago

As long as I can mark part of the caption with , sure.

As for the margin, you just need to add a CSS rule that says something like

figure.figure figcaption p:last-child {
  margin-bottom: 0;
}
fsbraun commented 1 year ago

What do you want to mark part of the caption with? Of course, you can always adjust a site's css in a project. 👍 I just do not want to make customized css part of djangcms-frontend.

greyhare commented 1 year ago

<cite>

Apparently GitHub doesn't like it without the back ticks.

greyhare commented 1 year ago

And sure, you don't want to ship a bunch of customization, but you also don't want the default case to look crappy so someone has to make a djangocms-frontend-fix-the-css project.

Maybe have some spot in the docs for recommended CSS?

fsbraun commented 1 year ago

FIxed by #66 😃