Skip to content

Conversation

@afbora
Copy link
Member

@afbora afbora commented Sep 10, 2019

Describe the PR

@distantnative Need your review for this simple PR :))

Fixed Preview Field

fields:
  perPage:
    label: Einträge pro Seite
    type: range
    step: 3
    min: 3
    max: 30
    default: 12
    required: true
    translate: false

FireShot Capture 746 - Test - Mægazine - http___localhost_test_kirby_1445_panel_pages_notes+test

Related issues

Todos

  • Add unit tests for fixed bug/feature
  • Pass all unit tests
  • Fix code style issues with CS fixer and composer fix
  • If needed, in-code documentation (DocBlocks etc.)

@distantnative
Copy link
Member

It looks good to me but I haven't been able to test it myself.

@distantnative distantnative removed the request for review from bastianallgeier September 10, 2019 10:25
@afbora afbora marked this pull request as ready for review September 10, 2019 10:27
@bastianallgeier bastianallgeier merged commit ad4abcb into getkirby:develop Sep 10, 2019
@bastianallgeier
Copy link
Member

I made a few small changes. The computed position value should be used in all places. Then it works as expected.

@bastianallgeier bastianallgeier added this to the 3.2.5 milestone Sep 10, 2019
@afbora afbora deleted the fix/1445-range-field-default-value branch September 11, 2019 21:28
@plan44
Copy link

plan44 commented Mar 31, 2020

Sorry for posting here but it seems closely related:
For a range with min=0 and max=5, when the stored value is 0, it does not display the value zero (neither on the slider nor in the label) but the default value.

It seems to me that this is a JS zero/null confusion caused by the code changes above.

The original line 80 in RangeInput.vue explicitly compared with null, where the change now only uses this.value which is falseish also when it contains the number zero.

I tried to test/fix it in my kirby installation but apparently the vue sources are not directly used but need some build step I have no idea how to perform.

@afbora afbora mentioned this pull request Mar 31, 2020
4 tasks
@afbora
Copy link
Member Author

afbora commented Mar 31, 2020

Thank you @plan44, i have fixed the issue on #2549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants