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

New props and types export #98

Merged
merged 5 commits into from
Jul 27, 2022

Conversation

gnekoz
Copy link
Contributor

@gnekoz gnekoz commented Jul 19, 2022

  • Modified the Link component adding the href prop
  • Exported the LabelFactoryProps type related to the Select component

beawar and others added 4 commits July 7, 2022 14:36
Set the dependency to only windowObj instead of windowObj.document.activeElement
in order to avoid the hook to re-run each time the activeElement change.
This change was causing for modals a reset of the focused element to the modal itself,
making the input inside losing focus while typing.

refs: CDS-54
…-devel

fix: avoid contents inside modal to lose focus on change of active node
@beawar
Copy link
Contributor

beawar commented Jul 25, 2022

Regarding the Link component typing, I'm not convinced by this solution. I think we could simplify the definition of this component removing the props already defined in Text, and defining it as an extension of the AnchorElement to accept also other attributes of the anchor tag.

It could become something like

type LinkProps = {
  /** Whether the link should be underlined */
  underlined?: boolean;
} & React.AnchorHTMLAttributes<HTMLAnchorElement> &
  TextProps;

Particularly, I don't like this line where you need to declare href on the styled component:
https://github.com/Zextras/carbonio-design-system/blob/227e81c8bb3fea9131a1a122a7d16444b7c8f1b1/src/components/basic/Link.tsx#L16-L19

That section of attrs should be used only for props which are declared dynamically by some condition or for props with a fixed value (like the forwardedAs, which should be always "a" for this component)

I tried to make these changes to see if the href is accepted by typescript when using a Link, see this patch
link-types.zip

@gnekoz
Copy link
Contributor Author

gnekoz commented Jul 26, 2022

I agree on the new definition of LinkProps and I confirm it's working without raising TS errors. I just pushed the patched version

@beawar beawar requested a review from nubsthead July 26, 2022 14:18
@beawar beawar merged commit 025fa48 into CDS-24-design-system-demo-review Jul 27, 2022
@beawar beawar deleted the new-props-and-types-export branch July 27, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants