silverstripe / silverstripe-admin

Silverstripe Admin Component
BSD 3-Clause "New" or "Revised" License
25 stars 94 forks source link

Unable to enter a number > 999 in Elemental NumericField #1575

Open elliot-sawyer opened 1 year ago

elliot-sawyer commented 1 year ago

Using Silverstripe 5.1.0-beta1 / Elemental tag 5.0.3

I'm trying to add a NumericField to an Element, using the following code:

private static $db = [
    'Width' => 'Int',
];

$fields->addFieldToTab('Root.Main', NumericField::create('Width'));

When I enter a value of 999 using inline editing, the value saves correctly. Anything over that, the value gets saved into the database but does not show in the frontend field. If I set inline_editable = false, the fields load correctly. As a work around, I've used a TextField instead and ensured my values are cast to an int during onBeforeWrite(); You can also use inline_editable = false;

Acceptance criteria

GuySartorelli commented 1 year ago

Thank you for reporting this.

Moving this to the admin repo since it will almost certainly be caused by the react implementation for this field provided by that module.

Would you like to submit a pull request to fix this bug?

emteknetnz commented 1 year ago

This also affects 4.13

Issue is that the react field cannot handle the thousands separately, e.g. 999 is fine while 1,000 is not

You can also workaround this by simply setting ->setHTML5(true) which will remove any numeric formatting

There's a bunch of custom php code in NumericField.php to do fancy localisation stuff which works fine if your using an regular old NumericField <input type="text"> in on a page edit form that does server side validation, there you can do things like enter european formatted numbers like 10 000

However the fancy custom localisation all seems to fall apart on the react fields

We could write a bunch of custom JS code to get the react field to client-side localisation validation ... buuut this seems like a real uphill battle and not a good use of time

Seems like we should really be doing something closer to setting everything to html5 and removing our own custom logic and relying on the browser to do the work here. There's a bunch of good stuff that happens by using the correct input.type attribute such as switching the keyboard that shows on mobile devices. I really don't think we should pursuing a solution that relies on type="text" and custom logic.

GuySartorelli commented 1 year ago

As an aside, we should probably have html5 set to true by default for CMS 6 - any probably even remove that method entirely.

elliot-sawyer commented 1 year ago

I believe I tried ->setHTML5(true) but the issue remains when using inline editing

elliot-sawyer commented 1 year ago

@GuySartorelli unfortunately I don't have the React skills needed to make a pull request to fix it myself

emteknetnz commented 1 year ago

the issue remains when using inline editing

That's not the experience I had in both CMS 4 and 5 - ->setHTML5(true) resolved the issue for me

elliot-sawyer commented 1 year ago

No worries, I'll take your word for it! I may not have used the parameter or set it to false

maxime-rainville commented 1 year ago

If you stick this snippet in NumericField, the fields works fine.

    public function getSchemaStateDefaults()
    {
        $state = parent::getSchemaStateDefaults();
        $state['value'] = $this->dataValue();
        return $state;
    }

Basically our React NumberField doesn't have way to honour the format. When it receives a value like 1,234, it doesn't know what to do with it.

By overriding the getSchemaStateDefaults, you bypass the problem by making sure you get the raw value when rendering the field through a form schema.

It's a bit hackish, but it solves the immediate problem. We can create a follow card to address this in CMS 6 the proper way.

lekoala commented 8 months ago

Maybe related https://github.com/silverstripe/silverstripe-framework/issues/11162