Skip to content
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

Fixing Error in Hds::Flyout & Hds::Modal subcomponents #2511

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zamoore
Copy link
Contributor

@zamoore zamoore commented Oct 18, 2024

📌 Summary

Fixes an error in the deprecated HdsDialogPrimitive::Header/Body/Description/Footer components as well as the Hds::Modal::Body component. Passes args from constructor call to super within constructor.

🛠️ Detailed description

I discovered an error caused by the recent TS conversion. When we converted the Hds::Flyout::Body component, we incorrectly passed {} as the args argument to super in the constructor, when we should use the argument that is passed from the constructor. To make that clearer, here's an example:

Code causing error:

export interface Signature {
  Blocks: {
    default: [];
  };
  Element: HTMLDivElement;
}

export default class MyComponent extends Component<Signature> {
  constructor(owner: unknown) {
    // object is passed to super, args not utilized from constructor
    super(owner, {});
  }
}

Should be

export interface Signature {
  Blocks: {
    default: [];
  };
  Element: HTMLDivElement;
}

export default class MyComponent extends Component<Signature> {
  constructor(owner: unknown, args: Record<string, never>) {
    super(owner, args);
  }
}

🔗 External links

Slack discussion


💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Oct 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Oct 21, 2024 7:40pm
hds-website ✅ Ready (Inspect) Visit Preview Oct 21, 2024 7:40pm

@zamoore zamoore force-pushed the fix-missing-constructor-args branch from 8cdaf9d to e93a689 Compare October 21, 2024 15:33
@zamoore zamoore changed the title Fixing Error in HdsFlyoutHeader Fixing Error in Hds::Flyout & Hds::Modal subcomponents Oct 21, 2024
@zamoore zamoore marked this pull request as ready for review October 21, 2024 16:07
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor suggestion (feel free to ignore or edit as you see fit).

packages/components/src/components/hds/flyout/body.ts Outdated Show resolved Hide resolved
.changeset/dull-lamps-retire.md Outdated Show resolved Hide resolved
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constructor changes look good. the signature adjustments in flyout/modal subcomponents (the deprecated body, description, footer, header) are not needed. the contextualClasses are only used on the dialog-primitive subcomponents

packages/components/src/components/hds/flyout/body.ts Outdated Show resolved Hide resolved
Co-authored-by: Cristiano Rastelli <[email protected]>
@@ -7,15 +7,16 @@ import Component from '@glimmer/component';
import { deprecate } from '@ember/debug';

export interface HdsFlyoutBodySignature {
Args: never;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] as a pattern, do we prefer to have Args completely missing (I think I have done something like this, recently), or Args: never like here? which one is more "correct™"?

/cc @alex-ju

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is it because below we need to have a HdsFlyoutBodySignature['Args']?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] as a pattern, do we prefer to have Args completely missing (I think I have done something like this, recently), or Args: never like here? which one is more "correct™"?

not having Args is the established pattern. however, considering the error we encounter in HCP for this subcomponent, I don't see a way of avoiding never.

we could use never straight in the constructor, but we have this pattern of using the signature in constructors, so what we have in this PR feels more consistent.

constructor(owner: unknown, args: never) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not having Args is the established pattern

@alex-ju Good. So nothing to change on my side :)

however, considering the error we encounter in HCP for this subcomponent, I don't see a way of avoiding never.

@alex-ju @zamoore since this is a deviation from the pattern, should we add a comment above these Args: never are not following the established pattern? (and potentially have also a link to this thread, for future reference)

Comment on lines +5 to +11
`Hds::DialogPrimitive`
- Fixed error in `Description` and `Body` subcomponents, by not passing the `args` argument from the constructor to `super`
- Added missing arguments in `Arg` TypeScript signature object

`Hds::Modal`
- Fixed error in `Body` subcomponent, caused by not passing the `args` argument from the constructor to `super`
- Added missing arguments in `Arg` TypeScript signature object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog would need to be updated, as we're not making any changes to the DialogPrimitive and there are no missing arguments anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants