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: default labels with SettingDropdown and SelectDropdown #3854

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

dsevillamartin
Copy link
Member

Changes proposed in this pull request:

  • I've removed the array check that was reverted to in fix: empty string displayed as SelectDropdown title #3773. The code if (Array.isArray(label)) label = label[0]; is from 2015. I don't think either that or extractText is needed here -- I tested this in v1 first with no issues in the same examples provided in that PR.

    https://github.com/flarum/framework/blame/f7dd609b264d4266151efa3b8a4c5775526346fc/framework/core/js/src/common/components/SelectDropdown.js#L48
    image

  • Made the defaultLabel not override custom ones set by vnode attrs. This fixes the fallbacks in PermissionGrid for eg. discussion & tag renaming. They're currently broken in v1 as well.

    The previous change was needed because otherwise it'd only show "For" (since trans returns an array). Mithril can handle arrays just fine - the label[0] looks to me to be from when Mithril perhaps would join with commas (I vaguely remember that?)

  • Also translated the "Custom" fallback. Wasn't sure what key to give the translation, since SettingDropdown is only used in PermissionGrid (but technically could be used anywhere in admin, I believe).

Screenshot

Before After
image image

QA

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.

@dsevillamartin dsevillamartin requested a review from a team as a code owner July 13, 2023 16:16
@SychO9 SychO9 changed the title Fix default labels with SettingDropdown and SelectDropdown fix: default labels with SettingDropdown and SelectDropdown Jul 27, 2023
@SychO9 SychO9 merged commit da1aa2a into 2.x Jul 27, 2023
9 of 19 checks passed
@SychO9 SychO9 deleted the ds/fix-dropdowns-default-label branch July 27, 2023 10:24
@SychO9 SychO9 added this to the 2.0 milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants