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

fix: Listing block default summary should use ul tag for its listing #6318

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ana-oprea
Copy link
Member

This PR continues the work from PR #4994, which is now outdated. To ensure progress on the development, I've opened this new PR to carry the changes forward and address any remaining tasks or updates.

Copy link

netlify bot commented Sep 25, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit a2f6a28
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66f52e549a8bf800080b2b7c

@ichim-david
Copy link
Member

@plone/volto-accessibility we need to have a look at this change and provide feedback

@ichim-david
Copy link
Member

@JeffersonBledsoe never mind the review request there needs to be some changes done to this pull request, one of them being not reintroducing Semantic-ui component within the Summary Template

@ichim-david
Copy link
Member

ichim-david commented Sep 25, 2024

@ana-oprea there are a couple of issues currently:

  1. the listing gets empty p tag even when no content is found (no description added for item)
  2. the listing also affects the coloring which might only be of importance for editors and otherwise
    confusing for anon users and wonder why there are blue or black titles
  3. it's using again semantic ui react which we want to avoid adding back into core and would be better
    to use simple html elements
  4. It's fair that we should avoid using headings for the listing even though classic uses h2, it should be
    something like .listing-header

review-state
classic-summary-view

@avoinea avoinea added the 99 tag: UX Accessibility Accessibility issues label Sep 26, 2024
@sneridagh
Copy link
Member

@ana-oprea Thanks for removing SemanticUI fromt here! However, I won't remove the current .ui.list CSS, since you could break a lot of projects out there.
Just add the new one instead. Also, we need an upgrade guide entry since it's a breaking change in HTML of the component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
99 tag: UX Accessibility Accessibility issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants