-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(rollouts/rules): UI for rollout/rule editing of segments #1953
Changes from all commits
ca80185
fddcf1c
0c3af94
a4c37a3
0eb7379
c612516
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,34 +1,45 @@ | ||||
import { MinusSmallIcon } from '@heroicons/react/24/outline'; | ||||
import { useRef } from 'react'; | ||||
import { MinusSmallIcon, PlusSmallIcon } from '@heroicons/react/24/outline'; | ||||
import { useRef, useState } from 'react'; | ||||
import { twMerge } from 'tailwind-merge'; | ||||
import Combobox from '~/components/forms/Combobox'; | ||||
import { FilterableSegment, ISegment } from '~/types/Segment'; | ||||
import { truncateKey } from '~/utils/helpers'; | ||||
|
||||
type SegmentPickerProps = { | ||||
readonly?: boolean; | ||||
editMode?: boolean; | ||||
segments: ISegment[]; | ||||
selectedSegments: FilterableSegment[]; | ||||
segmentAdd: (segment: FilterableSegment) => void; | ||||
segmentReplace: (index: number, segment: FilterableSegment) => void; | ||||
segmentRemove: (index: number) => void; | ||||
}; | ||||
|
||||
export default function SegmentsPicker(props: SegmentPickerProps) { | ||||
const { | ||||
segmentAdd, | ||||
selectedSegments: parentSegments, | ||||
segments, | ||||
segmentRemove, | ||||
segmentReplace | ||||
} = props; | ||||
export default function SegmentsPicker({ | ||||
readonly = false, | ||||
editMode = false, | ||||
markphelps marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
segments, | ||||
selectedSegments: parentSegments, | ||||
segmentAdd, | ||||
segmentReplace, | ||||
segmentRemove | ||||
}: SegmentPickerProps) { | ||||
const segmentsSet = useRef<Set<string>>( | ||||
new Set<string>(parentSegments.map((s) => s.key)) | ||||
); | ||||
|
||||
const segmentsSet = useRef<Set<string>>(new Set<string>()); | ||||
const [editing, setEditing] = useState<boolean>(editMode); | ||||
|
||||
const handleSegmentRemove = (index: number) => { | ||||
const filterableSegment = parentSegments[index]; | ||||
|
||||
// Remove references to the segment that is being deleted. | ||||
segmentsSet.current!.delete(filterableSegment.key); | ||||
segmentRemove(index); | ||||
|
||||
if (editMode && parentSegments.length == 1) { | ||||
setEditing(true); | ||||
} | ||||
}; | ||||
|
||||
const handleSegmentSelected = ( | ||||
|
@@ -68,43 +79,70 @@ export default function SegmentsPicker(props: SegmentPickerProps) { | |||
filterValue: truncateKey(s.key), | ||||
displayValue: s.name | ||||
}))} | ||||
disabled={readonly} | ||||
selected={selectedSegment} | ||||
setSelected={(filterableSegment) => { | ||||
handleSegmentSelected(index, filterableSegment); | ||||
}} | ||||
inputClassName={ | ||||
readonly | ||||
? 'cursor-not-allowed bg-gray-100 text-gray-500' | ||||
: undefined | ||||
} | ||||
/> | ||||
</div> | ||||
<div> | ||||
<button | ||||
type="button" | ||||
className="text-gray-400 mt-2 hover:text-gray-500" | ||||
onClick={() => handleSegmentRemove(index)} | ||||
> | ||||
<MinusSmallIcon className="h-6 w-6" aria-hidden="true" /> | ||||
</button> | ||||
</div> | ||||
{editing && parentSegments.length - 1 === index ? ( | ||||
<div> | ||||
<button | ||||
type="button" | ||||
className={twMerge(` | ||||
text-gray-400 mt-2 hover:text-gray-500 ${ | ||||
readonly ? 'hover:text-gray-400' : undefined | ||||
}`)} | ||||
onClick={() => setEditing(false)} | ||||
title={readonly ? 'Not allowed in Read-Only mode' : undefined} | ||||
disabled={readonly} | ||||
> | ||||
<PlusSmallIcon className="h-6 w-6" aria-hidden="true" /> | ||||
</button> | ||||
</div> | ||||
) : ( | ||||
<div> | ||||
<button | ||||
type="button" | ||||
className="text-gray-400 mt-2 hover:text-gray-500" | ||||
onClick={() => handleSegmentRemove(index)} | ||||
title={readonly ? 'Not allowed in Read-Only mode' : undefined} | ||||
disabled={readonly} | ||||
> | ||||
<MinusSmallIcon className="h-6 w-6" aria-hidden="true" /> | ||||
</button> | ||||
</div> | ||||
)} | ||||
</div> | ||||
))} | ||||
<div className="w-full"> | ||||
<div className="w-5/6"> | ||||
<Combobox<FilterableSegment> | ||||
id={`segmentKey-${parentSegments.length}`} | ||||
name={`segmentKey-${parentSegments.length}`} | ||||
placeholder="Select or search for a segment" | ||||
values={segments | ||||
.filter((s) => !segmentsSet.current!.has(s.key)) | ||||
.map((s) => ({ | ||||
...s, | ||||
filterValue: truncateKey(s.key), | ||||
displayValue: s.name | ||||
}))} | ||||
selected={null} | ||||
setSelected={(filterableSegment) => { | ||||
handleSegmentSelected(parentSegments.length, filterableSegment); | ||||
}} | ||||
/> | ||||
{(!editing || parentSegments.length === 0) && ( | ||||
<div className="w-full"> | ||||
<div className="w-5/6"> | ||||
<Combobox<FilterableSegment> | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean like this?
Or something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yah, nvm i see that the above is only rendered when in 'editing' mode |
||||
id={`segmentKey-${parentSegments.length}`} | ||||
name={`segmentKey-${parentSegments.length}`} | ||||
placeholder="Select or search for a segment" | ||||
values={segments | ||||
.filter((s) => !segmentsSet.current!.has(s.key)) | ||||
.map((s) => ({ | ||||
...s, | ||||
filterValue: truncateKey(s.key), | ||||
displayValue: s.name | ||||
}))} | ||||
selected={null} | ||||
setSelected={(filterableSegment) => { | ||||
handleSegmentSelected(parentSegments.length, filterableSegment); | ||||
}} | ||||
/> | ||||
</div> | ||||
</div> | ||||
</div> | ||||
)} | ||||
</div> | ||||
); | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does
twMerge
replace theclassNames
util helper?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markphelps I think so? Was the
classNames
util helper for handling overrides of tailwind styles as well? If so, it didn't seem to be working for me for some reason 🤔. This article explains whytwMerge
should be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think
twMerge
is like the functionality ofclassNames
+ handles tailwind specifically.classNames
is just a way to dynamically apply classes given a boolean condition. so it might make sense for us to favortwMerge
instead ofclassNames
in the future as it has the added ability of working with tw classes like we might expect (last one wins).So maybe the rule should be, if we find ourselves needing to use
classNames + twMerge
then just usetwMerge
as it can do both I think?