Updates a few remaining places where we check constraint operators with
the new constraint operator checks. Additionally, because there was only
one remaining place where we used the `oneOf` function, I replaced it
with a normal `includes` check and deleted the `oneOf` util. From what I
can tell, there's no need to have that utility function; it doesn't
provide much benefit over using the language built-ins 🤷🏼
Doesn't clear the value from the constraint input value popover if you
close it and then re-open. In other words, if you accidentally click
out, you don't lose your progress. Instead, the popover will open again,
with the value you had when you closed it highlighted (so that it's easy
to type over if you want to):
<img width="452" alt="image"
src="https://github.com/user-attachments/assets/d86aa00e-4956-40a8-8fea-e75be5d5425b"
/>
The reason I'm changing this now is because I noticed that the error
wasn't cleared correctly when the popover was closed. If we do it this
way instead, then that makes sense, because you can still see the value.
This is also how the single-value popover has worked forever.
From some quick testing, the single value popover still works as
expected:
<img width="562" alt="image"
src="https://github.com/user-attachments/assets/9041a922-b055-4310-ab60-93ad219981a4"
/>
As a side note: I'm adding a comment to anyone coming after as to why
focus handling on escape doesn't work correctly on the single value
button. I was about to go down a rabbit hole on that before I read my
own comment on the previous PR. So I thought I'd put that here too.
Makes it so that the constraint value input gives you an error if you
try to add one or more values that **all** exist in the set of values
already. E.g. if you have `a` and `b`, and try to add `a`, it'll tell
you that "`a` has already been added". Likewise, if you try to add
`a,b`, it'll tell you that all these values already exist. However, if
at least one of the values does not exist, then it will allow you to
submit the values (we already do deduplication before storing anyway).
The background for this is that a user was confused thinking that just
one specific value didn't get added to their constraints. As it turns
out, they'd already added the value previously, so when it didn't show
up at the end of the list, they thought it didn't work at all.
<img width="863" alt="image"
src="https://github.com/user-attachments/assets/12195e0a-04bc-4b41-bd44-432120c768a6"
/>
<img width="816" alt="image"
src="https://github.com/user-attachments/assets/433a64d7-aec0-482d-8544-574656c266ce"
/>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR updates how we show old/new strategy/segment names in change
requests, and also removes the name of the strategy type from the
change.
For the old/new names: instead of showing them stacked vertically, we
show them side by side (old name first, then new name).
Compare before:
<img width="967" alt="image"
src="https://github.com/user-attachments/assets/d3e36f49-4abc-4cd4-8ba9-752515740185"
/>
with after:
<img width="974" alt="image"
src="https://github.com/user-attachments/assets/d0f85264-b055-4c44-b985-f992f09d8dab"
/>
Only affects the new components (legacy CR view is untouched). If we get
negative feedback while rolling it out because the strat type name is
missing, we can always add it back.
The max width is set to `max(40vw, 1000px)`, so that if you're on a very
wide window, then it'll take up at most 40% of the horizontal space.
Once your window is smaller than 2500px, however, the sidebar will stop
shrinking and stay at 1000px (or as close to that as the window allows).
It'll keep shrinking with the window size.
This came up because in certain cases, such as if you have a release
template with a long description, the modal would just keep growing
until it took up 98% of the window width.
I have not set a min width for now. I don't think there is any need for
it, but if we find there is, we can add it back later.
Before:
After:
Adds "change:" to the beginning of all changes and does some work to
align the use of compononents and structure across them (supersedes
https://github.com/Unleash/unleash/pull/10260).
In doing so, I have also added new and legacy variants for all different
change components, because this has required some hierarchy
restructuring every now and then. A reason for doing that was adding the
correct wrapping behavior for components, such that on smaller screens,
we wouldn't entirely blow out and make the kebab menu invisible and
inaccessible.
It also makes it so that we switch to full-width change view earlier (at
breakpoint md instead of sm), because at sm, a lot of stuff got hidden
before we switched to full-width.
Most changes are trivial updates; I've called out bits of the code that
are not in comments.
Rendered, it looks like this:
<img width="1203" alt="image"
src="https://github.com/user-attachments/assets/36bed974-99da-4d8d-a881-ea9df7797210"
/>
One interesting and potentially quite useful side-effect, is that all
change types now use the exact same set of components in the same
fashion, as evidenced by this screenie where I've added outlines to the
hierarchy:
<img width="1020" alt="image"
src="https://github.com/user-attachments/assets/685fefcc-af7e-4697-b8f3-8260af1e2a84"
/>
The one difference is that components without a diff place the "more"
kebab menu one layer further inside to facilitate prettier wrapping (the
kebab menu can stay on the same line as the other text when wrapping):
<img width="238" alt="image"
src="https://github.com/user-attachments/assets/2b8d3174-06a8-4ad4-b366-cea97720deda"
/>
Removes the view diff hover links (and strategy icons) in the new views
and removes trailing colons.
In removing the hover links, I have split up the content of their files
(`StrategyTooltipLink` and `SegmentTooltipLink`) into
`Change{Segment|Strategy}Name` and `{Segment|Strategy}Diff`. I have
reverted the existing tooltip files to their state before I began
changing this and added deprecation notices. These old tooltips are only
used by the old components, so we don't need to work on them after all.
In doing this work, I've also updated the strategy change diff to handle
the new functionality (tabs instead of hover)
The removal of the trailing colons (so that it's `adding strategy
gradual rollout` instead of `adding strategy: gradual rollout` is in
preparation for the remaining changes to the header that we're
introducing with this change. The removal of these is not behind a flag,
so I've also done it in the legacy components. This feels like a very
low risk change, so it felt like more work to have to check a flag for
each of the different instances where we use it.
<img width="839" alt="image"
src="https://github.com/user-attachments/assets/b29b8073-c282-4b4b-948f-9a545082ac31"
/>
We don't want deleted strategies to have the red background color
anymore with the new change/diff view. As such, we can remove it in the
new component and add a todo to delete the css for it after it's gone.
<img width="1050" alt="image"
src="https://github.com/user-attachments/assets/083cbcac-5df9-47cd-bd78-2501c3bbf64e"
/>
Updates the strategy change component used in change requests to have a
"tab"-like functionality, where you can switch between "Change" (the
rendered strategy) and "View diff" (the JSON diff). This is a tab
switcher, so you navigate it with arrow keys if on a keyboard, and I've
set selection to follow focus for now. This is my preference as it saves
you extra space/enter taps, but we can remove it later if we want.
Most of the changes in this PR is wrapping the existing strategy change
components/sections in tab panels and tab context providers.
Later changes in this project will remove the existing "view diff" hover
link in favor of this view.
There is some work to do on the design front (in terms of spacing,
borders, etc), but this covers the core functionality.
The pre-existing strategy change component has been moved into the
"LegacyStrategyChange" file with no changes beyond a deprecation
comment. The existing tests now test the new component instead with no
breakage. (I anticipate breaking when we remove the view diff link,
though)
Change with strategy variants:
<img width="991" alt="image"
src="https://github.com/user-attachments/assets/ac28912f-5b08-4a9c-96da-81bfd0b2e68e"
/>
<img width="1005" alt="image"
src="https://github.com/user-attachments/assets/4addaacc-101c-46cb-888f-95dc3b1cac25"
/>
## Edge case: deleted strat with no reference strategy
There is a case in the code where "reference strategy" can be undefined
for a deleted strategy. It is defined as follows:
```ts
const referenceStrategy =
changeRequestState === 'Applied'
? change.payload.snapshot
: currentStrategy;
```
I've decided to still show the "(no changes)" json diff in that setting,
so that the tabs actually affect something. I don't know how often this
happens, though: probably not anymore unless your CR is _ancient_, as we
introduced the snapshot quite a while ago, and `currentStrategy`
shouldn't really be undefined here, I should think.
Deleted strategy with no reference strategy:
Change:
<img width="1013" alt="image"
src="https://github.com/user-attachments/assets/352eaec9-c0ef-4d5a-b765-11304daf4474"
/>
Diff:
<img width="1029" alt="image"
src="https://github.com/user-attachments/assets/e69c81a6-1ef7-47ff-853a-9fb900b26303"
/>
Follow-up to: https://github.com/Unleash/unleash/pull/10213
This makes our `AddValuesPopover` backdrop click-through. This means you
can interact with any element in the "background" right away and it will
work, while closing the popover at the same time.
If this works well it may be worth extracting to a reusable
ClickThroughPopover or similar.
Adds a number of small corrections and fixes to (mainly) CR-related
component. Discovered as part of adding the new JSON diff view. Refer to
the various inline comments for explanations.
---------
Co-authored-by: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com>
This change adds the `crDiffView` flag to Unleash, potentially enabling
the new JSON diff tab in change request changes instead of the "view
json diff" hover functionality.
This PR takes two steps towards better constraint handling:
## New type: `IConstraintWithId`
Introduces a new type, `IConstraintWithId`. This is the same as an
`IConstraint`, except the constraint id property is required. The idea
is that the list of editable constraints should move towards using this
instead of just `IConstraint`. That should prevent us (on a type-level)
from seeing more of the same kind of errors we saw with the segment
constraints yesterday.
I don't want to go ahead and update all the upstream uses of this to
IConstraintWithId in this PR, so I'll look at that separately.
## API payload constraint replacer
Introduces an api payload constraint "replacer", which we can use for
[JSON.stringify's `replacer`
parameter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#the_replacer_parameter).
The current implementation works both for strategies and for segments
and has been added to edit + create forms for both of these resources.
This has a couple benefits:
1. We can clearly state exactly how we want them to be rendered,
including property order. I've decided to go with context -> operator ->
value(s) as the main one (check the screenie), as I believe this is the
most logical reading order.
2. We can exclude value/values (whichever one doesn't work with the
operator)
3. It doesn't matter how we treat constraints internally, we can still
present the payload how we want
4. Importantly: this only affects the stringification for the
user-facing API payload, so it's very low risk. It does not affect
anything that we actually send to the api.
Here's what it can look like with ordered properties:
<img width="392" alt="image"
src="https://github.com/user-attachments/assets/f46f77c8-0b5a-4ded-b13a-bb567df60bd3"
/>
Prevents the property order from changing when constraints are set from
the editable constraint component. When we render out the API command,
we don't specify the order of properties in the objects, which means
that it can change dramatically, which can be a little jarring.
As it is right now, it first renders in one order when you open the
strategy form:
<img width="299" alt="image"
src="https://github.com/user-attachments/assets/52cf2445-d9eb-402c-b5bc-0fece5fbe822"
/>
And when you navigate to the targeting section of the strategy form, it
changes to:
<img width="299" alt="image"
src="https://github.com/user-attachments/assets/e4cb7006-dcf4-4e88-befb-ccba5b647ddd"
/>
This also applies to constraints in segments.
With this change, the order will remain the same before and after.
Additionally, there's some extra tests around constraint ids being kept
the same and being set if it doesn't exist.
## Further work
This came about as part of the issue we had with constraint editing in
segments being broken as of now. As part of that, I would like to make
some further improvements (such as making the ID required when you use a
list of editable constraints), but that will be in a follow-up. There
are some complications that might not make it a viable option, sadly.
We could also try to stabilize the property order in the API rendering
methods (which I think might be a good idea), but because there's
multiple different ones, this seems to be a faster solution.
This adds constraint ids to segment constraints used in editing
segments. Without them, there was a bug where when you went to edit the
segment, all constraints would be invisibly set to the same constraint.
Addresses and removes all leftover comments related to the flag overview
redesign flag.
There's four changes here:
1. Remove release plan milestone strategy and environment footer.
Dangling files, no references.
2. Delete old code without references in theme.ts
3. Delete legacy playground result chip. Replace all references to it
with references to the new chip. The API is the exact same and the
legacy chip was just passing through everything to the new chip anyway,
so nothing should change.
4. Remove a now-redundant way to supply a default
Deletes the legacy strategy separator and removes all references to it.
Luckily, all references to the separator were in dangling files that
could themselves be deleted directly.
This PR addresses and removes the last comment related to the
addEditStrategy flag. In doing so, I have also removed the remaining
dangling files from the new constraint accordion directory. I believe
that everything that's left in there now is currently in use.
