Feat: Introduce alignment to Dropdown component #DS-1411 #1837

crishpeen · 2025-01-08T12:09:01Z

Description

We need this for Dropdown in Header Navigation.

Additional context

Issue reference

netlify · 2025-01-08T12:09:17Z

✅ Deploy Preview for spirit-design-system ready!

Name	Link
🔨 Latest commit	`a500b28`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system/deploys/677e859007479e00088d64e4
😎 Deploy Preview	https://deploy-preview-1837--spirit-design-system.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 96 (no change from production) Accessibility: 91 (no change from production) Best Practices: 100 (no change from production) SEO: 91 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-01-08T12:11:31Z

✅ Deploy Preview for spirit-design-system-storybook ready!

Name	Link
🔨 Latest commit	`6d3becd`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system-storybook/deploys/677e6adfcd55470008a457d6
😎 Deploy Preview	https://deploy-preview-1837--spirit-design-system-storybook.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

coveralls · 2025-01-08T12:14:49Z

coverage: 79.469%. remained the same
when pulling a43b7c7 on feature/dropdown-alignment
into fc5ceb3 on integration/header.

netlify · 2025-01-08T12:18:21Z

✅ Deploy Preview for spirit-design-system-storybook canceled.

Name	Link
🔨 Latest commit	`a500b28`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system-storybook/deploys/677e8590bba5d5000857bb65

github-actions · 2025-01-08T12:23:48Z

Here is the URL of the uploaded artifact: https://github.com/lmc-eu/spirit-design-system/actions/runs/12670287379/artifacts/2401164873

curdaj · 2025-01-09T10:43:43Z

packages/web-react/src/components/Dropdown/__tests__/Dropdown.test.tsx


    fireEvent.click(trigger);

    expect(onToggle).toHaveBeenCalled();
  });
+
+  it('should render with horizontal alignment', () => {


Maybe it would be more efficient via dataProvider and it.each?

curdaj · 2025-01-09T10:46:25Z

packages/web-react/src/components/Dropdown/demo/DropdownAlignment.tsx

+          <Item elementType="a" href="#" label="Something else here" />
+        </DropdownPopover>
+      </Dropdown>
+      <div className="px-800 py-1700 bg-tertiary text-center">This a big unrelated box</div>


Box component? :D

adamkudrna

I'd only like to clarify the nullability of alignment, otherwise ✅ for me.

adamkudrna · 2025-01-09T13:39:01Z

packages/web/src/scss/components/Dropdown/index.html

+
+  <div class="Container">
+
+    <h2 class="docs-Heading">Alignment</h2>


It's rather an edge-case prop, but I'd consider placing this demo right after the "Placement" section.

adamkudrna · 2025-01-09T13:39:38Z

packages/web/src/scss/components/Dropdown/index.html

+          </div>
+        </div>
+        <div class="px-800 py-1700 bg-tertiary text-center">
+          This a big unrelated box


Suggested change

This a big unrelated box

This is a big unrelated box to demonstrate the alignment

or

Suggested change

This a big unrelated box

This is a big unrelated box to demonstrate the Dropdown Trigger alignment

adamkudrna · 2025-01-09T13:46:01Z

packages/web/src/scss/components/Dropdown/README.md

@@ -254,6 +254,38 @@ See the [Item][item] component for more information.
 DropdownPopover implements the [Placement Dictionary][dictionary-placement] for placement. The dictionary values are used as
 a value of data attribute `data-spirit-placement`, e.g. `data-spirit-placement="top"`, `data-spirit-placement="right-end"`, etc.

+## Alignment
+
+Dropdown supports the extended [Alignment Dictionary][dictionary-alignment] for alignment on both axis. To use it, add the


Plural?

Suggested change

Dropdown supports the extended [Alignment Dictionary][dictionary-alignment] for alignment on both axis. To use it, add the

Dropdown supports the extended [Alignment Dictionary][dictionary-alignment] for alignment on both axes. To use it, add the

adamkudrna · 2025-01-09T13:58:16Z

packages/web/src/scss/components/Dropdown/README.md

+specific class to the `.Dropdown` element, e.g. `.Dropdown--alignmentXRight` or `.Dropdown--alignmentYStretch`. Adding
+any of these classes will make the element display as `flex`.
+
+We also support responsive infix for alignment classes. To use them, add the infix to the class name, e.g. `.Dropdown--tablet--alignmentXRight`.


infixes … them ?

adamkudrna · 2025-01-09T14:02:32Z

packages/web-react/src/components/Dropdown/README.md

+| `alignmentX`      | \[ [AlignmentXExtended dictionary][dictionary-alignment] \| `object`] | `null`         | ✕        | Apply vertical alignment to trigger, use object to set responsive values, e.g. `{ mobile: 'left', tablet: 'center', desktop: 'right' }`   |
+| `alignmentY`      | \[ [AlignmentYExtended dictionary][dictionary-alignment] \| `object`] | `null`         | ✕        | Apply horizontal alignment to trigger, use object to set responsive values, e.g. `{ mobile: 'top', tablet: 'center', desktop: 'bottom' }` |


Are the props intentionally nullable? Do we have it like this in other components with alignment?

adamkudrna · 2025-01-09T14:06:35Z

packages/web/src/scss/components/Dropdown/_Dropdown.scss

+.Dropdown[class*='--alignment'] {
+    display: flex;
+}


This relates to my question about nullable alignment: if you only define alignment for some breakpoints, display flex will be applied in all of them. Is it safe? 🤔

adamkudrna · 2025-01-09T14:08:42Z

packages/web-twig/src/Resources/components/Dropdown/Dropdown.twig

-{%- set _classNames = [ _rootClassName, _styleProps.className ] -%}
+
+{%- set _alignmentXClasses = [] -%}
+{%- if _alignmentX is iterable and _alignmentX is not null -%}


Is the second condition necessary? Can an iterable variable be null? 🤔

adamkudrna · 2025-01-09T14:11:09Z

packages/web-react/src/types/dropdown.ts

+import {
+  AlignmentXExtendedDictionaryType,
+  AlignmentYExtendedDictionaryType,
+  Booleanish,
+  ChildrenProps,
+  ClickEvent,
+  PlacementDictionaryType,
+  StyleProps,
+} from './shared';


Sigh: If Prettier didn't change formatting here, we could quickly see which imports are new… 🤷🏻‍♂️

We can still change the line width to 80 chars :-) But base on 🔤 order, first two ;-)

adamkudrna · 2025-01-09T14:13:43Z

packages/web-react/src/components/Dropdown/useDropdownStyleProps.ts

-import { useClassNamePrefix } from '../../hooks';
-import { DropdownStyleProps } from '../../types';
+import { useAlignmentClass, useClassNamePrefix } from '../../hooks';
+import { DropdownStyleProps, GridAlignmentXType, GridAlignmentYType } from '../../types';


Grid alignment types? Is it correct in Dropdown? 🤔

I think this needs to be refactored.

literat · 2025-01-10T12:29:44Z

packages/web-react/src/types/dropdown.ts

@@ -18,11 +26,23 @@ export type DropdownTriggerRenderProps = {
  ref: LegacyRef<HTMLButtonElement & HTMLAnchorElement>;
 };

-export interface DropdownProps extends ChildrenProps, StyleProps {
+export type DropdownAlignmentXType =


suggestion (if-minor): If DropdownAlignment and the GridAlignment dictionaries/types are the same, maybe we should refactor them and not copy and rename them.

literat · 2025-01-10T12:33:34Z

Are we able to present this feature a little bit better, please? This looks like a bug, IMHO.

crishpeen added the run-visual-tests Runs visual regression testing on this PR label Jan 8, 2025

crishpeen requested review from literat, adamkudrna, pavelklibani, a team and curdaj as code owners January 8, 2025 12:09

github-actions bot assigned crishpeen Jan 8, 2025

github-actions bot added the feature New feature or request label Jan 8, 2025

crishpeen force-pushed the integration/header branch from 0005622 to fc5ceb3 Compare January 8, 2025 12:10

crishpeen force-pushed the feature/dropdown-alignment branch from 6d3becd to a43b7c7 Compare January 8, 2025 12:11

crishpeen added 3 commits January 8, 2025 15:02

Feat(web): Introduce alignment to Dropdown component #DS-1411

4a6958f

Feat(web-react): Introduce alignment to Dropdown component #DS-1411

ba75c82

Feat(web-twig): Introduce alignment to Dropdown component #DS-1411

a500b28

crishpeen force-pushed the feature/dropdown-alignment branch from a43b7c7 to a500b28 Compare January 8, 2025 14:02

curdaj approved these changes Jan 9, 2025

View reviewed changes

adamkudrna approved these changes Jan 9, 2025

View reviewed changes

literat reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Introduce alignment to Dropdown component #DS-1411 #1837

Feat: Introduce alignment to Dropdown component #DS-1411 #1837

crishpeen commented Jan 8, 2025

netlify bot commented Jan 8, 2025 •

edited

Loading

netlify bot commented Jan 8, 2025 •

edited

Loading

coveralls commented Jan 8, 2025 •

edited

Loading

netlify bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025

curdaj Jan 9, 2025

curdaj Jan 9, 2025

adamkudrna left a comment

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

adamkudrna Jan 9, 2025

literat Jan 10, 2025

adamkudrna Jan 9, 2025

literat Jan 10, 2025

literat Jan 10, 2025

literat commented Jan 10, 2025


		<div class="Container">

		<h2 class="docs-Heading">Alignment</h2>

	This a big unrelated box
	This is a big unrelated box to demonstrate the alignment

	Dropdown supports the extended [Alignment Dictionary][dictionary-alignment] for alignment on both axis. To use it, add the
	Dropdown supports the extended [Alignment Dictionary][dictionary-alignment] for alignment on both axes. To use it, add the

		\| `alignmentX` \| \[ [AlignmentXExtended dictionary][dictionary-alignment] \\| `object`] \| `null` \| ✕ \| Apply vertical alignment to trigger, use object to set responsive values, e.g. `{ mobile: 'left', tablet: 'center', desktop: 'right' }` \|
		\| `alignmentY` \| \[ [AlignmentYExtended dictionary][dictionary-alignment] \\| `object`] \| `null` \| ✕ \| Apply horizontal alignment to trigger, use object to set responsive values, e.g. `{ mobile: 'top', tablet: 'center', desktop: 'bottom' }` \|

Feat: Introduce alignment to Dropdown component #DS-1411 #1837

Are you sure you want to change the base?

Feat: Introduce alignment to Dropdown component #DS-1411 #1837

Conversation

crishpeen commented Jan 8, 2025

Description

Additional context

Issue reference

netlify bot commented Jan 8, 2025 • edited Loading

✅ Deploy Preview for spirit-design-system ready!

netlify bot commented Jan 8, 2025 • edited Loading

✅ Deploy Preview for spirit-design-system-storybook ready!

coveralls commented Jan 8, 2025 • edited Loading

netlify bot commented Jan 8, 2025 • edited Loading

✅ Deploy Preview for spirit-design-system-storybook canceled.

github-actions bot commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkudrna left a 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

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

literat commented Jan 10, 2025

netlify bot commented Jan 8, 2025 •

edited

Loading

netlify bot commented Jan 8, 2025 •

edited

Loading

coveralls commented Jan 8, 2025 •

edited

Loading

netlify bot commented Jan 8, 2025 •

edited

Loading