Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

OcSelect: add support for showing default values #1634

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented Sep 4, 2021

(depends on #1633)

Description

I've added an optionaldefaultValue prop to OcSelect.
The value provided there is selected when value === null. When the default value is shown the clear button is hidden because you obviously can't clear the default.
It's possible to select the default value which then fires the input event, i.e. a user can override a default value by explicitly setting it.

I'm not really happy with the usability of this as it is right now for multiple reasons:

  1. clicking an x and then seeing some value is confusing
  2. clicking an x and then still seeing the same value is even more confusing
    (when the default was explicitly selected by the user)
  3. it's not obvious what the default value is as soon as something is selected, c.f. placeholder for OcTextInput and OcTextarea

I figured it might be a bit better if we would use a different icon for "reverting to default", e.g. something like the replay icon provided by ODS. Unfortunately vue-select does not seem to provide api to replace the clear button: https://github.com/sagalbot/vue-select/blob/4230bba9349c5e3ba33a4e77a566fe3ff7d2e1c5/src/components/Select.vue#L58

If we decide to somehow display the default outside of the actual select component we need to make sure we respect label and possibly other relevant props.

We need to decide if we want to settle on clearButtonEnabled or clearable (...or whatever) as name for this kind of property.
vue-select uses clearable, OcTextInput uses clearButtonEnabled. I'm fine with whatever, although I personally like clearable a bit better I chose clearButtonEnabled for not having to introduce a breaking change in OcTextInput...

Related Issue

Motivation and Context

I want to show a default value in ownBrander 2.0 if a user hasn't selected a value yet.

It's possible to handle this outside of the OcSelect component, but that's rather cumbersome and dealing with the vue-select events seem a bit brittle, which is why I would prefer to have this solved for once in ODS. With the additional benefit that my code wouldn't depend on vue-select and vue-select could be replaced better (if neccessary one day).

Moreover, this needs to be thought through UX wise and having a solution in ODS would avoid different approaches in different places.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Showing the default:
Screenshot_20210905_004208
Default selected explicitly:
Screenshot_20210905_004257

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation added/updated

Open tasks:

  • ...

@dschmidt dschmidt force-pushed the work/select_defaultValue branch from 0860c53 to 910a002 Compare September 4, 2021 23:26
@sonarcloud
Copy link

sonarcloud bot commented Sep 4, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

7.4% 7.4% Coverage
0.0% 0.0% Duplication

<div class="oc-docs-width-medium">
<oc-select v-model="selected" :options="['Apple', 'Bannana', 'Orange', 'Pear']" default-value="Orange" />
<p>
Selected: {{ selected === null ? "null" : selected }}
Copy link
Member Author

@dschmidt dschmidt Sep 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Selected: {{ selected === null ? "null" : selected }}
Selected: {{ selected || "null" }}

</template>
</vue-select>
<div>
<label :for="id" v-text="label" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<label :for="id" v-text="label" />
<label :for="id" class="oc-label" v-text="label" />

Just checked how the update journey in web would look like and we seem to give all labels this class, which doesn't style anything initially but sometimes is used to target the label for styling

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense but possibly belongs into the original pr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but that one is merged already (and I somehow ended here without understanding this is not the original ;D my bad, could you include it either way?

@@ -128,6 +172,23 @@ export default {
*/
this.$emit("search:input", event.target.value)
},
onInput(value) {
// selecting a value is handled via onOptionSelecting, we just nead to handle clear events here
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*need

return listeners
},
showClearButton() {
return this.clearButtonEnabled && this.value !== null
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.disabled needs to be taken into account.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants