Sage / carbon

Carbon by Sage | ReactJS UI Component Library
https://carbon.sage.com
Apache License 2.0
277 stars 86 forks source link

`className` prop on `DialogFullscreen` has no effect #6187

Open clheppell opened 1 year ago

clheppell commented 1 year ago

Current behaviour

If I set a className prop on a DialogFullscreen, the className is never passed down to anything in the DOM.

I'm told by one of our QAs that this works on a normal Dialog.

Expected behaviour

Either the value to be passed down to the DOM or the prop not to be documented in the Storybook and TypeScript definitions.

CodeSandbox or Storybook URL

https://codesandbox.io/s/friendly-kalam-3kx8w6?file=/src/App.js

JIRA Ticket (Sage Only)

No response

Suggested Solution

No response

Carbon Version

118.2.1

Design Tokens Version

No response

What browsers are you seeing the problem on?

Firefox, Chrome

What Operating System are you seeing the problem on?

Windows

Anything else we should know?

No response

Confidentiality

Parsium commented 1 year ago

Thanks for this @clheppell, may I ask what your use case is for using className? If this is for testing purposes, then we can introduce a data-* tag on the DialogFullscreen's container instead.

I believe it is a mistake that className is still documented as a prop for DialogFullscreen, as due to problems in the past, we don't recommend using it.

clheppell commented 1 year ago

We're not using className ourselves, but we are using (and extending) Carbon's TypeScript types to power our Sage 200 Storybook which will be made available to third party developers who write addons and customisations for our web forms.

Our QAs tested whether they can use all the props listed in our documentation and found that this one doesn't work. We're currently removing this prop from our documentation by using TypeScript's Omit utility type, and raised this bug so that the prop is either removed from the types or is fixed.

For what it's worth, we see no reason for the prop to exist.

For context, we use our own Storybook because

  1. Sage 200 web forms are amended using a JSON schema, so we write our own component examples.
  2. We wrap all Carbon components and have some of our own. This allows us to document our own props in addition to the Carbon ones and to document our own components and have them all in one place.
Parsium commented 1 year ago

Ah makes sense thanks for the context 👍🏼 We do need to go back and removing consumer support for className generally in all our components. I'll get this on our backlog. In the meantime, if you are able to omit the prop when constructing your own types, I would continue doing this for now 😄

Parsium commented 1 year ago

FE-6066