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

feat: allow use of any tag in listItems helper #3147

Merged
merged 5 commits into from
Nov 8, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Nov 2, 2021

Changes proposed in this pull request:

This PR allows the use of any tag with the listItems helper function. This is useful to easily map an ItemList into separate DOM nodes outside of a <ul> element, without the need of code duplication.

Not everything needs to be a list. In fact, using <ul> and <li> too much is a common issue across the web, and is the reason why WebKit (Safari on macOS and iOS) actually remove all semantics from list tags when their list-style is set to none. (Source)

This change could help to prevent such "list-itis" within Flarum and its extensions.

Reviewers should focus on:

Screenshot

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@davwheat davwheat merged commit ec730d2 into master Nov 8, 2021
@davwheat davwheat deleted the dw/listitems-custom-tag branch November 8, 2021 23:52
@askvortsov1 askvortsov1 added this to the 1.2 milestone Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants