feat: update select dropdown to match multiselect one #516

f-necas · 2023-07-05T13:10:54Z

An overlay has been set to be as closed as the multiselect dropdown.

Sometimes the selected value is a string or an object. I made the majority of the changes in the dropdown component in order to keep the original behavior of each parent component.

github-actions · 2023-07-05T13:13:04Z

Affected libs: feature-dataviz, feature-record, feature-router, feature-editor, ui-catalog, feature-catalog, ui-inputs, feature-search, feature-map, ui-elements, ui-search,
Affected apps: datafeeder, metadata-editor, datahub, demo, webcomponents, search, metadata-converter, map-viewer,

🚀 Build and deploy storybook and demo on GitHub Pages
📦 Build and push affected docker images

tkohr

Thanks @f-necas for the big work! The visual result is really nice and identical to the multi-dropdown.

However, the app currently breaks if choice is undefined in the dropdown-selector. Can easily be tested with records without data for the previews. Also noticed two minor issues in the UI:

The values in the orgs sort now wrap. Do you think this could be prevented?
The sort label is not properly aligned on mobile.

I've left corresponding comments inline.

libs/feature/dataviz/src/lib/feature-dataviz.module.ts

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.ts

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.html

f-necas · 2023-07-17T08:10:06Z

Thanks for your review @tkohr. I've updated the code in order to :

Display only a part of choice label if it's too long (I've seen in my dataset a long text which overflow the entire page)
Set a min-width in order to have a correct display for sort by. (I could take the longest label but it may be related to the first point (very long select) and setting a whitespace-nowrap in choices labels overflows the overlay)
Set the number of max rows as constant
Put a conditional display if choices isn't set. ( I think we won't need the select if there's no value ?)

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.html

tkohr · 2023-07-18T07:51:44Z

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.ts

+    return (
+      this.selectedChoice.label.substring(0, 50) +
+      (this.selectedChoice.label.length > 50 ? '...' : '')
+    )


Can't we achieve this with a truncate via CSS?

I have done a truncate in css BUT couldn't set parent as flex in map-view component. When external link is displayed , it's not on the same line. Couldn't figure it out why flex is breaking the css truncate thing.

min-w-0 is also mandatory to have the truncate working...

I haven't followed the discussion but TW provide a truncate class for ellipsis. It's not about flex.
https://tailwindcss.com/docs/text-overflow#truncate

I haven't followed the discussion but TW provide a truncate class for ellipsis. It's not about flex. https://tailwindcss.com/docs/text-overflow#truncate

Yeah I know and use it already, but the flex class on a parent breaks his behaviour.

Hum, indeed it doesn't seem simple to make the cascaded truncate work within the flex here. A <div class="truncate"> wrapper directly under the flex helps to apply the truncate further down, but also cuts borders etc. because of the overflow-hidden.
We really need to keep the flex justify-end layout in the map-view.component, though, to keep a fitting design for short/normal dropdown entries.

@jahow @fgravin What do you think of it ?

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.html

jahow · 2023-07-27T13:42:21Z

libs/feature/dataviz/src/lib/chart-view/chart-view.component.stories.ts

@@ -21,6 +22,7 @@ export default {
      imports: [
        ChartComponent,
        HttpClientModule,
+        OverlayModule,


I think we should import the UiInputs module here instead, as OverlayModule is an underlying dependency

jahow · 2023-07-27T13:50:37Z

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.html

+      <input
+        class="w-[0px] h-[18px] align-text-top shrink-0"
+        type="text"
+        [checked]="isSelected(choice)"
+        (click)="onSelectValue(choice)"
+      />


this has no reason to be there; instead, of isSelected(choice) is true, the following span should be highlighted just like in a native select:

jahow · 2023-07-27T13:53:42Z

libs/feature/dataviz/src/lib/chart-view/chart-view.component.html

@@ -1,23 +1,25 @@
 <div class="w-full h-full flex flex-col">
  <div
-    class="flex flex-col space-y-2 sm:flex-row sm:space-y-0 sm:space-x-2 justify-between text-[13px]"
+    class="flex flex-col flex-wrap space-y-2 sm:flex-row sm:space-y-0 sm:space-x-2 justify-between text-[13px] gap-4"


In principle, when modifying a presentation component, we shouldn't have to modify anything that uses this component. Otherwise this indicates that the encapsulation is broken and that our changes are leaking "outside" of the modified component scope.

In that case I think it is acceptable to adjust options because the dropdown is a little bit larger, but modifying the layout (introducing flex-wrap etc.) is a too big change in my opinion.

jahow · 2023-07-27T15:06:01Z

Thanks for the extensive work on this @f-necas, this PR is clearly very close to be good to go. However as discussed I think we should refrain from merging it before shipping 1.1, as it might have unexpected side effects and is IMO not strictly required for the release. Sorry if that is disappointing 😬

coveralls · 2023-08-25T17:30:10Z

coverage: 81.642% (-4.3%) from 85.915% when pulling 1519aee on ui-sort-dropdown into a9fd646 on main.

* show selected item Also modified button: * removing minus margin and add hidden border to buttons

jahow

I think we're in a pretty good state now. @f-necas you can merge whenever you feel that the PR is ready! Thanks for the continued effort.

f-necas marked this pull request as ready for review July 6, 2023 08:41

f-necas requested a review from jahow July 6, 2023 08:41

f-necas force-pushed the ui-sort-dropdown branch from 1f3eb6a to a49a7a7 Compare July 6, 2023 10:47

f-necas added enhancement Proposal for a new feature minor feature labels Jul 6, 2023

fgravin requested a review from tkohr July 13, 2023 12:07

fgravin assigned tkohr Jul 13, 2023

tkohr reviewed Jul 13, 2023

View reviewed changes

f-necas force-pushed the ui-sort-dropdown branch 2 times, most recently from badf863 to 4a415f3 Compare July 17, 2023 08:03

f-necas requested a review from tkohr July 17, 2023 08:10

f-necas force-pushed the ui-sort-dropdown branch from 4a415f3 to f80282b Compare July 17, 2023 08:15

tkohr reviewed Jul 18, 2023

View reviewed changes

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.html Outdated Show resolved Hide resolved

tkohr reviewed Jul 18, 2023

View reviewed changes

libs/ui/inputs/src/lib/dropdown-selector/dropdown-selector.component.html Show resolved Hide resolved

f-necas requested a review from tkohr July 18, 2023 12:27

jahow reviewed Jul 27, 2023

View reviewed changes

jahow modified the milestones: 1.2.0, 2.0.0 Jul 27, 2023

f-necas force-pushed the ui-sort-dropdown branch from 8dfcad7 to 9a20665 Compare August 25, 2023 17:21

f-necas requested review from jahow and fgravin August 25, 2023 17:50

f-necas force-pushed the ui-sort-dropdown branch from af9465f to 519fc7a Compare August 31, 2023 09:04

f-necas removed request for fgravin and tkohr September 1, 2023 10:13

f-necas requested review from jahow and removed request for jahow September 1, 2023 10:14

f-necas force-pushed the ui-sort-dropdown branch from 519fc7a to 511fa37 Compare September 19, 2023 08:03

f-necas added 2 commits September 22, 2023 11:08

feat: update select dropdown to match multiselect one

f008fdd

feat: add keyboard support

73a774f

jahow force-pushed the ui-sort-dropdown branch from e848206 to 6451801 Compare September 22, 2023 09:12

feat(ui): improvements to dropdown

1519aee

* show selected item Also modified button: * removing minus margin and add hidden border to buttons

jahow force-pushed the ui-sort-dropdown branch from 6451801 to 1519aee Compare September 22, 2023 09:13

jahow approved these changes Sep 22, 2023

View reviewed changes

f-necas merged commit f491bdb into main Sep 22, 2023
5 checks passed

jahow deleted the ui-sort-dropdown branch October 19, 2023 08:23

tkohr mentioned this pull request Oct 20, 2023

Fix [datahub]: Display external viewer button correctly #656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update select dropdown to match multiselect one #516

feat: update select dropdown to match multiselect one #516

f-necas commented Jul 5, 2023

github-actions bot commented Jul 5, 2023 •

edited

Loading

tkohr left a comment •

edited

Loading

f-necas commented Jul 17, 2023

tkohr Jul 18, 2023

f-necas Jul 18, 2023

fgravin Jul 18, 2023

f-necas Jul 18, 2023

tkohr Jul 20, 2023

f-necas Jul 24, 2023

jahow Jul 27, 2023

jahow Jul 27, 2023

jahow Jul 27, 2023

jahow commented Jul 27, 2023

coveralls commented Aug 25, 2023 •

edited

Loading

jahow left a comment

feat: update select dropdown to match multiselect one #516

feat: update select dropdown to match multiselect one #516

Conversation

f-necas commented Jul 5, 2023

github-actions bot commented Jul 5, 2023 • edited Loading

tkohr left a comment • edited Loading

Choose a reason for hiding this comment

f-necas commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahow commented Jul 27, 2023

coveralls commented Aug 25, 2023 • edited Loading

jahow left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 5, 2023 •

edited

Loading

tkohr left a comment •

edited

Loading

coveralls commented Aug 25, 2023 •

edited

Loading