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

Allow form select to show specific text for empty option that is not placeholder option… #13763

Merged
merged 5 commits into from Dec 10, 2018

Conversation

diosmosis
Copy link
Member

… & add missing translation.

image

Changes:

  • In select form-fields', if there is already an '' empty option, use that in the dropdown. This still allows showing the placeholder if the value is set to, eg, null or undefined).
  • Add a 'Show all' entry to the access level filter dropdowns.
  • Add a missing translation.

Fixes #13698

@Findus23 what do you think? Would this make selecting all sites clearer to you?

The placeholder font color is theme related so won't address in this issue. Though if it's a problem, perhaps a separate issue is called for.

…placeholder option & add missing translation.
@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Nov 25, 2018
@diosmosis diosmosis added this to the 3.8.0 milestone Nov 25, 2018
@tsteur
Copy link
Member

tsteur commented Nov 26, 2018

fyi it worked for me and code looks 👍 .

From a UX point of view it might be worth thinking about having some content above this saying something like "Give access to all websites [permission selector] [confirm button]". Then we could also add a note that user will get only access to currently existing websites, not to websites created in the future. But we could always add this later in a later release as it doesn't seem too important maybe.

@diosmosis
Copy link
Member Author

@tsteur added the addition you suggested to the component

@tsteur
Copy link
Member

tsteur commented Dec 9, 2018

Works 👍 and nice to use. feel free to merge once tests pass

@Findus23
Copy link
Member

Findus23 commented Dec 9, 2018

@diosmosis Thanks for the fix.

@diosmosis diosmosis merged commit 7956ca7 into 3.x-dev Dec 10, 2018
@diosmosis diosmosis deleted the 13698-user-edit-filter branch December 10, 2018 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants