Add Support for customTarget in MultiSelect Component #6915

donghee214 · 2024-07-17T20:05:50Z

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

Support customTarget in MultiSelect, and use that as the target

Reviewers should focus on:

MultiSelect functionality is mostly retained even when using customTarget. Recognize we will lose the clear all button if customTarget is enabled. Can possibly look into this as a followup, but will need some design brainstorming to come up with a suitable location for this button within the Popover

Screenshot

changelog-app · 2024-07-17T20:05:56Z

Generate changelog in `packages/docs-app/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Add Support for children in MultiSelect Component

Generate changelog in `packages/select/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Add Support for children in MultiSelect Component

Check the box to generate changelog(s)

Generate changelog entry

svc-palantir-github · 2024-07-17T20:18:03Z

finish feature

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github · 2024-07-17T20:38:51Z

clean up PR

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

packages/select/src/components/multi-select/_multi-select.scss

svc-palantir-github · 2024-07-17T21:00:02Z

focus on opening

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

donghee214 · 2024-07-17T21:10:13Z

packages/select/src/components/multi-select/_multi-select.scss

+    // offset the padding added by the multiselect component so menu can remain full width with padding on either side
+    // this is important so menu dividers can be full width, and so that menu items can have padding to the left and right
+    // for menu items
+    margin-left: -$select-padding;
+    margin-right: -$select-padding;


Confirmed this doesn't break anything with existing MenuDividers

Current State (without the negative margins and without popover-content padding):

New State (with the negative margins and with popover-content padding):

we may need to test with enabling the scrollbar to always be visible in your device settings - we had an issue recently related to this which I fixed here: #6804

though it looks like you're doing the same thing as Select so I expect this to be good

leaving note for followup, this appears in Select as well - would rather just keep this consistent with Select for now and not try to change both in this PR

note 5px above and below scrollbar

this PR

current:

packages/select/src/components/multi-select/multiSelect.tsx

svc-palantir-github · 2024-07-17T21:53:02Z

Add generated changelog entries

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

…print into dkim/custom-target-popover

svc-palantir-github · 2024-07-17T22:23:58Z

Merge branch 'dkim/custom-target-popover' of github.com:palantir/blueprint into dkim/custom-target-popover

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson

looking good! leaving some initial comments for now

packages/docs-app/src/styles/_examples.scss

packages/select/src/__examples__/multiSelectCustomTarget.tsx

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx

evansjohnson · 2024-07-17T21:59:22Z

packages/select/src/components/multi-select/_multi-select.scss

+    // offset the padding added by the multiselect component so menu can remain full width with padding on either side
+    // this is important so menu dividers can be full width, and so that menu items can have padding to the left and right
+    // for menu items
+    margin-left: -$select-padding;
+    margin-right: -$select-padding;


we may need to test with enabling the scrollbar to always be visible in your device settings - we had an issue recently related to this which I fixed here: #6804

though it looks like you're doing the same thing as Select so I expect this to be good

packages/select/src/components/multi-select/multiSelect.tsx

evansjohnson · 2024-07-17T22:23:39Z

packages/select/src/components/multi-select/multiSelect.tsx

+                        {/* If customTarget is provided, TagInput is not rendered. This loses the search input.
+                        To account for this, render the search input within the popover similar to Select.
+
+                        Clearing all items is still possible since this component is controlled. It just not cannot
+                        be through the default button in this component, rather done through a custom button that can be
+                        rendered from within the popover through the itemListRenderer or from externally. */}
+                        {this.props.customTarget != null && input}


highlighting this as a TODO - let's catch up on this once everything else is looking good

packages/select/src/components/multi-select/multiSelect.tsx

svc-palantir-github · 2024-07-18T06:07:34Z

update tests

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github · 2024-07-18T07:16:03Z

address PR comments

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github · 2024-07-18T07:47:41Z

lint and update test

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson

main focus here on key event handlers/keyboard nav support and updating docs - a few of these comments are just notes not to action now so keep an eye out for that

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx

packages/select/src/components/multi-select/multiSelect.tsx

packages/docs-app/src/styles/_examples.scss

packages/select/src/components/multi-select/multiSelect.tsx

evansjohnson · 2024-07-18T14:18:32Z

packages/select/src/components/multi-select/_multi-select.scss

+    // offset the padding added by the multiselect component so menu can remain full width with padding on either side
+    // this is important so menu dividers can be full width, and so that menu items can have padding to the left and right
+    // for menu items
+    margin-left: -$select-padding;
+    margin-right: -$select-padding;


leaving note for followup, this appears in Select as well - would rather just keep this consistent with Select for now and not try to change both in this PR

note 5px above and below scrollbar

this PR

current:

packages/select/src/components/multi-select/multiSelect.tsx

packages/select/test/multiSelectTests.tsx

…print into dkim/custom-target-popover

svc-palantir-github · 2024-07-19T20:09:19Z

typo

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson

almost there 😄

packages/select/src/components/multi-select/multiSelect.tsx

evansjohnson · 2024-07-19T20:25:07Z

packages/select/src/components/multi-select/multiSelect.tsx

+            // Cannot rely on input to determine popover state as the input will be inside the Popover
+            // if customTarget is provided


Suggested change

// Cannot rely on input to determine popover state as the input will be inside the Popover

// if customTarget is provided

I think this comment only makes sense right now if you assume relying on the input is the "correct" way - I don't think we need to explain handling this separately. If anything the input focus handling could be explained, but I'd rather not just slap a comment on there now since we may miss some nuance about why it was done that way.

sounds good!

packages/select/test/multiSelectTests.tsx

svc-palantir-github · 2024-07-19T20:34:37Z

remove commented code

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github · 2024-07-22T14:38:26Z

disable open on keydown for customTarget

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

gluxon · 2024-07-24T18:34:10Z

Small visual nitpick: The box shadow on the <TagInput> is cut off by the solid white background of the list below. One way to fix this would be to add some spacing.

shadow.cutoff.mov

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx

svc-palantir-github · 2024-07-24T19:29:41Z

add margin bottom for box shadow

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson · 2024-07-24T21:27:48Z

packages/select/src/components/multi-select/multiSelect.tsx

@@ -303,7 +304,7 @@ export class MultiSelect<T> extends AbstractPureComponent<MultiSelectProps<T>, M
                    ref,
                    role: "combobox",
                },
-                this.props.customTarget != null ? this.props.customTarget : this.getTagInput(listProps),
+                this.props.renderTarget != null ? this.props.renderTarget(targetProps) : this.getTagInput(listProps),


I don't think we want to pass targetProps since we're already wrapping the custom target in an element that is handling spreading those.

I think the idea is to have a renderTarget callback that is passed selectedItems to ensure we always are rendering an up to date target. We could also use that callback to pass anything else that could be helpful, such as passing isOpen if the user would want to style differently based on that without separately keeping track of the loading state.

svc-palantir-github · 2024-07-24T21:37:39Z

update tests

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson

I think it may be better to revert before the last change and keep the customTarget naming, while still updating the API style per last comment. The comment was to create a renderTarget style API, but I'm not sure we actually want to reuse that name since we won't be threading through the props that are on Popovers renderTarget which may be unexpected given the name.

packages/docs-app/src/examples/select-examples/multiSelectCustomTarget.tsx

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx

svc-palantir-github · 2024-07-25T02:41:32Z

add margin bottom for box shadow

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github · 2024-07-25T03:19:32Z

revert to name customTarget

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson

thanks for the great contribution and working through a few changes in direction and getting some tests added!

finish feature

056f681

clean up PR

1af3196

focus on opening

afcc98a

donghee214 commented Jul 17, 2024

View reviewed changes

packages/select/src/components/multi-select/_multi-select.scss Show resolved Hide resolved

donghee214 added 2 commits July 17, 2024 17:03

udpate docs

7849428

update some more docs

8836958

donghee214 commented Jul 17, 2024

View reviewed changes

typos

8c03d07

donghee214 commented Jul 17, 2024

View reviewed changes

packages/select/src/components/multi-select/multiSelect.tsx Outdated Show resolved Hide resolved

donghee214 marked this pull request as ready for review July 17, 2024 21:39

Add generated changelog entries

2a259da

donghee214 assigned evansjohnson Jul 17, 2024

palantir deleted a comment from donghee214 Jul 17, 2024

donghee214 added 2 commits July 17, 2024 18:09

rename to custom target to avoid breaking change

7c5cf60

Merge branch 'dkim/custom-target-popover' of github.com:palantir/blue…

d0f4700

…print into dkim/custom-target-popover

evansjohnson reviewed Jul 17, 2024

View reviewed changes

update tests

1d35f95

address PR comments

b1c728e

lint and update test

a8969ac

evansjohnson reviewed Jul 18, 2024

View reviewed changes

donghee214 changed the title ~~Add Support for children in MultiSelect Component~~ Add Support for customTarget in MultiSelect Component Jul 18, 2024

donghee214 added 4 commits July 19, 2024 15:52

address PR

de56c48

Merge branch 'dkim/custom-target-popover' of github.com:palantir/blue…

5700074

…print into dkim/custom-target-popover

remove outdated comment

6bd3cdc

typo

3f52836

donghee214 added 2 commits July 19, 2024 16:20

lint

e5641d4

remove commented code

4813cb8

evansjohnson reviewed Jul 19, 2024

View reviewed changes

donghee214 added 3 commits July 22, 2024 10:10

address PR

b0e7d8a

re-enable tag props in exmaple

69a3428

disable open on keydown for customTarget

6a0a70e

donghee214 requested a review from evansjohnson July 22, 2024 14:28

add margin bottom for box shadow

6af45db

gluxon reviewed Jul 24, 2024

View reviewed changes

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx Outdated Show resolved Hide resolved

evansjohnson reviewed Jul 24, 2024

View reviewed changes

packages/docs-app/src/examples/select-examples/multiSelectCustomTarget.tsx Outdated Show resolved Hide resolved

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx Show resolved Hide resolved

donghee214 force-pushed the dkim/custom-target-popover branch from ff3bc18 to 6af45db Compare July 25, 2024 02:26

revert to name customTarget

116a437

donghee214 requested review from evansjohnson and gluxon July 25, 2024 03:11

evansjohnson approved these changes Jul 25, 2024

View reviewed changes

donghee214 merged commit d51c90c into develop Jul 25, 2024
13 checks passed

donghee214 deleted the dkim/custom-target-popover branch July 25, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for customTarget in MultiSelect Component #6915

Add Support for customTarget in MultiSelect Component #6915

donghee214 commented Jul 17, 2024 •

edited

Loading

changelog-app bot commented Jul 17, 2024 •

edited by donghee214

Loading

svc-palantir-github commented Jul 17, 2024

svc-palantir-github commented Jul 17, 2024

svc-palantir-github commented Jul 17, 2024

donghee214 Jul 17, 2024

evansjohnson Jul 17, 2024

evansjohnson Jul 18, 2024

svc-palantir-github commented Jul 17, 2024

svc-palantir-github commented Jul 17, 2024

evansjohnson left a comment

evansjohnson Jul 17, 2024

evansjohnson Jul 17, 2024

svc-palantir-github commented Jul 18, 2024

svc-palantir-github commented Jul 18, 2024

svc-palantir-github commented Jul 18, 2024

evansjohnson left a comment

evansjohnson Jul 18, 2024

svc-palantir-github commented Jul 19, 2024

evansjohnson left a comment

evansjohnson Jul 19, 2024

donghee214 Jul 22, 2024

svc-palantir-github commented Jul 19, 2024

svc-palantir-github commented Jul 22, 2024

gluxon commented Jul 24, 2024 •

edited

Loading

svc-palantir-github commented Jul 24, 2024

evansjohnson Jul 24, 2024

svc-palantir-github commented Jul 24, 2024

evansjohnson left a comment •

edited

Loading

svc-palantir-github commented Jul 25, 2024

svc-palantir-github commented Jul 25, 2024

evansjohnson left a comment

		// Cannot rely on input to determine popover state as the input will be inside the Popover
		// if customTarget is provided

Add Support for customTarget in MultiSelect Component #6915

Add Support for customTarget in MultiSelect Component #6915

Conversation

donghee214 commented Jul 17, 2024 • edited Loading

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

changelog-app bot commented Jul 17, 2024 • edited by donghee214 Loading

Generate changelog in packages/docs-app/changelog/@unreleased

Generate changelog in packages/select/changelog/@unreleased

svc-palantir-github commented Jul 17, 2024

finish feature

svc-palantir-github commented Jul 17, 2024

clean up PR

svc-palantir-github commented Jul 17, 2024

focus on opening

Choose a reason for hiding this comment

Current State (without the negative margins and without popover-content padding):

New State (with the negative margins and with popover-content padding):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-palantir-github commented Jul 17, 2024

Add generated changelog entries

svc-palantir-github commented Jul 17, 2024

Merge branch 'dkim/custom-target-popover' of github.com:palantir/blueprint into dkim/custom-target-popover

evansjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-palantir-github commented Jul 18, 2024

update tests

svc-palantir-github commented Jul 18, 2024

address PR comments

svc-palantir-github commented Jul 18, 2024

lint and update test

evansjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-palantir-github commented Jul 19, 2024

typo

evansjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-palantir-github commented Jul 19, 2024

remove commented code

svc-palantir-github commented Jul 22, 2024

disable open on keydown for customTarget

gluxon commented Jul 24, 2024 • edited Loading

svc-palantir-github commented Jul 24, 2024

add margin bottom for box shadow

Choose a reason for hiding this comment

svc-palantir-github commented Jul 24, 2024

update tests

evansjohnson left a comment • edited Loading

Choose a reason for hiding this comment

svc-palantir-github commented Jul 25, 2024

add margin bottom for box shadow

svc-palantir-github commented Jul 25, 2024

revert to name customTarget

evansjohnson left a comment

Choose a reason for hiding this comment

donghee214 commented Jul 17, 2024 •

edited

Loading

changelog-app bot commented Jul 17, 2024 •

edited by donghee214

Loading

Generate changelog in `packages/docs-app/changelog/@unreleased`

Generate changelog in `packages/select/changelog/@unreleased`

gluxon commented Jul 24, 2024 •

edited

Loading

evansjohnson left a comment •

edited

Loading