- Deletes an unused file
- Fixes a console.error due to a prop being passed on to the HTML
component
- Puts all editable constraint files within a separate directory.
We're migrating to ESM, which will allow us to import the latest
versions of our dependencies.
Co-Authored-By: Christopher Kolstad <chriswk@getunleash.io>
Uses a purple color for the hover underline. Also, sets it to be
transparent when not-hovered, so that you get a nice fade in effect.
Focus (top) and hover (bottom) now have the same visual style, but
different ways to get to that state (expansion vs fade-in):
<img width="979" alt="image"
src="https://github.com/user-attachments/assets/e342ea4e-4821-4e4c-bb5d-6b9d3a672e26"
/>
There can be any number of these on a page, so setting autofocus here is
a Bad Idea (TM). It's probably a holdover from when the input was an
accordion and we wanted to give you focus when you opened it (we do a
similar thing for the popover, for instance).
This property will cause you to focus (and potentially scroll) to the
last constraint on the page.
This PR implements a number of strategies to make the app perform better
when you have large lists. For instance, we have a constraint field that
has ~600 legal values. Currently in main, it is pretty slow and sloggy
to use (about on par with what we see in hosted).
With this PR, it becomes pretty snappy, as shown in this video (should
hopefully be even better in production mode?):
https://www.loom.com/share/2e882bee25a3454a85bec7752e8252dc?sid=7786b22d-6c60-47e8-bd71-cc5f347c4e0f
The steps taken are:
1. Change the `useState` hook to instead use `useReducer`. The reason is
that its dispatch function is guaranteed to have a stable identity. This
lets us use it in memoized functions and components.
2. Because useReducer doesn't update the state variable until the next
render, we need to use `useEffect` to update the constraint when it has
actually updated instead of just calling it after the reducer.
3. Add a `toggle value` action and use that instead of checking whether
the value is equal or not inside an onChange function. If we were to
check the state of the value outside the reducer, the memoized function
would be re-evaluated every time value or values change, which would
result in more renders than necessary. By instead doing this kind of
checking inside the reducer, we can cache more aggressively.
4. Because the onChange function can now be memoized, we can memoize all
the legal value selector labels too. This is the real goal here, because
we don't need to re-render 600 components, because one of them was
checked.
One side effect of using useEffect to call `onUpdate` is that it will
also be called immediately when the hook is invoked the first time, but
it will be called with the same value as the constraint that was passed
in, so I don't think that's an issue.
Second: the `useEffect` call uses `localConstraint` directly as a dep
instead of stringifying it. I'm not sure why, but stringifying it makes
it not update correctly for legal values.
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Adds a test suite for the useEditableConstraint hook, attempting to test
all the parts of it that we can't test in isolation.
Plus: a few, small refactorings:
- Renames `onAutoSave` on `onUpdate` to better match `onDelete` (and
because autosave doesn't really mean anything anymore).
- Simplifies and collapses some types
Adds a fairly comprehensive test suite for the constraint reducer. I put
in all cases I thought were relevant.
As part of this, I discovered one bug, and changed two actions.
The bug was toggling on the wrong property when you tried to invert case
sensitivity.
The action changes are:
- rename "remove value from list" to "remove value"
- remove "set value" in favor of instead letting "add value(s)" work in
single-value constraints too.
Extracts and tests the implementation of the functions to get deleted
and invalid legal values.
This is pretty straightforward stuff and arguably not crucial, but it
was an easy place to start and I don't think it hurts to have these in
place anyway.
Adds an input form for single legal values (i.e. for number and semver
operators).
- Uses the `validator` for the constraint to check the list of legal
values and provides a list of "invalid legal values" for values that
don't pass validation.
- Values in the "invalid legal values" set are "disabled" when rendered
in the UI. Additionally, there's an extra bit of text that tells you
that values that aren't valid are disabled.
- Makes the legal values selector more generic and makes it adapt to
multi- or single-value selection based on input props. The external
interface is two separate components (to make it clearer that they are
different things and because their props don't line up perfectly).
Rendered:
<img width="957" alt="image"
src="https://github.com/user-attachments/assets/cd8d2f32-057d-4e31-8fd3-174676eeb65e"
/>
Still todo: testing deleted/invalid legal value detection. I'll do that
as part of the big testathon in a followup.
Fixes an issue where you would lose keyboard focus when interacting with
one of the input fields in the new editable constraint.
The fix was spinning out the inputs into their own separate component.
This prevents them from being re-rendered every time (or something idk)
which allows us to keep focus.
It also stops the popover for multi-value constraint operators from
closing after you've entered a value; allowing for faster entry of more
values (as was intended and as was how it functioned previously).
This (admittedly pretty big) PR removes a component layer, moves all
logic for updating constraint values into a single module, and dumbs
down other components.
The main changes are:
- EditableConstraintWrapper is gone. All the logic in there has been
moved into the new `useEditableConstraint` hook. Previously it was split
between the wrapper, editableConstraint itself, the legalValues
component.
- the `useEditableConstraint` hook accepts a constraint and a save
function and returns an editable version of that constraint, the
validator for input values, a function that accepts update commands,
and, when relevant, existing and deleted legal values.
- All the logic for updating a constraint now exists in the
`constraint-reducer` file. As a pure function, it'll be easy to unit
test pretty thoroughly to make sure all commands work as they should
(tests will come later)
- The legal values selector has been dumbed down consiberably as it no
longer needs to create its own internal weak map. The internal
representation of selected values is now a set, so any kind of lookup is
now constant time, which should remove the need for the extra layer of
abstraction.
## Discussion points
I know the reducer pattern isn't one we use a *lot* in Unleash, but I
found a couple examples of it in the front end and it's also quite
similar to how we handle state updates to change request states. I'd be
happy to find a different way to represent it if we can keep it in a
single, testable interface.
Semi-relatedly: I've exposed the actions to submit for the updates at
the moment, but we could map these to functions instead. It'd make
invocations a little easier (you wouldn't need to specify the action
yourself; only use the payload as a function arg if there is one), but
we'd end up doing more mapping to create them. I'm not sure it's worth
it, but I also don't mind if we do 💁🏼
Implements client-side validation of constraint values before you can
add them to a constraint.
I've removed the extra server-side validation that used to happen for
each specific constraint, because the surrounding form itself uses
server side validation to check every constraint every time there's a
change. This is what controls disabling the submit button etc.
I wanna make the next PR a bit of a followup cleanup now that it's
clearer what properties we do and don't need.
<img width="371" alt="image"
src="https://github.com/user-attachments/assets/7c98708f-fcbe-40ca-8590-bb0f5b2ad167"
/>
<img width="361" alt="image"
src="https://github.com/user-attachments/assets/503d4841-d910-4e8e-b0ef-a3d725739534"
/>
Adds inputmode='decimal' to input fields with number input. As discussed
on the [GOV.UK
blog](https://technology.blog.gov.uk/2020/02/24/why-the-gov-uk-design-system-team-changed-the-input-type-for-numbers/),
this finds a balance between giving numeric input options to mobile
devices and improving validation / user experience.
They mention this bit in their [design system
guideline](https://design-system.service.gov.uk/components/text-input/#numbers)
> Do not use `<input type="number">` unless your user research shows
that there’s a need for it. With `<input type="number">` there’s a risk
of users accidentally incrementing a number when they’re trying to do
something else - for example, scroll up or down the page. And if the
user tries to enter something that’s not a number, there’s no explicit
feedback about what they’re doing wrong.
I've purposefully not included the `pattern="[0-9]*"` attribute here,
because the browser error messages conflict with our own and have
several drawbacks in terms of accessibility according to Adrian
Roselli's ["Avoid default field
validation"](https://adrianroselli.com/2019/02/avoid-default-field-validation.html).
Instead, the validation here will be part of the validation handling
later.
Also, I've opted for using `decimal` instead of `numeric`, because we
allow you to store decimal values and that inputmode also adds the
decimal separator to the keyboard. As always, however, there's
complications: several languages (including Norwegian) use a comma as a
decimal separator instead of a period, so the keyboard will likely
contain numbers and a comma instead of a period. This is a problem
because JS doesn't recognize "45,6" as a valid number. I've added a
follow-up task to look into this. I thought at first it would just be
expanding the validation, but because it's stored as a string on the
back end and the SDKs presumably parse it, we can't just suddenly allow
commas as decimal separators.
