In MGT React add support for specifying CSS class name on elements

[waldekmastykarz]

Proposal: In MGT React add support for specifying CSS class name on elements

Description

In MGT React add support for specifying CSS class name on elements.

Rationale

This would allow developer to style components in a specific way that matches their application.

Preferred Solution

Additional Context

This issue is inspired by the need to style the list of events rendered by the agenda component with additional top and bottom margin as presented at https://github.com/pnp/sp-starter-kit/blob/master/documentation/components/wp-personal-calendar.md.

[ghost]

Hello waldekmastykarz, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[marcelwgn]

I am not sure if I understand this proposal correctly, do you want to be able to set the className property on MGT-React components and those classes should be applied to the actual webcomponent? In that case, that's already possible:

<Person personDetails={personDetails} view={PersonViewType.oneline} className="some-class"/>

yields following DOM entry:

<mgt-person class="some-class"></mgt-person>

[waldekmastykarz]

I believe I had errors in React when using className due to how components' inheritance was set up. Perhaps it's been fixed already.

[nmetulev]

I wonder if this works in jsx but not tsx? As in, maybe we just need to add the className property to the prop types for the components?

[marcelwgn]

@nmetulev Indeed, this is an issue with typescript usage, trying to assign a value to the className property gives the following error:

Type '{ className: string; }' is not assignable to type 'IntrinsicAttributes & PersonProps & { children?: ReactNode; }'.
  Property 'className' does not exist on type 'IntrinsicAttributes & PersonProps & { children?: ReactNode; }'.  TS2322

What would the appropriate way to fix this? Add that to the prop types of the mgt-react controls?

[nmetulev]

The simplest would be to add the prop in the script that generates the react props. We could add it here when the props object is instantiated: https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/main/packages/mgt-react/scripts/generate.js#L43

For example

props['className'] = 'string';

@chingucoding, do you fancy a PR?

[marcelwgn]

Sure, I'll create a PR!

[waldekmastykarz]

Rather than adding a custom className property, shouldn't we use it from React by changing the inheritance of our component classes?

[marcelwgn]

Good point, otherwise we would keep adding properties everytime someone is missing a react property. I can't think of a reason why the components shouldn't inherit from React.Component. Though, a possible issue is that not every property might make full sense to expose? For example the props property might not have any effect at all, exposing that as an API doesn't really make much sense then.

Edit: I am not entirely sure how we would have them inherit from React.Component as I am not too familiar with the code base. Just updating the props doesn't resolve the issue of missing className and even crashes for the mgt-Get element as it doesn't export the necessary methods needed for React.Component.

[waldekmastykarz]

Looking at it further, it seems like exposing the specific properties on the component is the way to go after all. HTML attributes are not exposed by default and if a component should allow setting a specific HTML property, it should expose it as a property. Sorry for the confusion.

[marcelwgn]

Right, I see; thank you for the clarification/update! While we are at it, do you guys think it would be better to add more than just className, for example id or aria-label?

[nmetulev]

Good idea - I think id is a good candidate to also include while you are adding className.

Can aria-* attributes be set as properties? If not, we'd have to update the wrapper to recognize them as such and set them as attributes instead. IMO, out of scope for this issue if that's the case, but we can open a new issue to track that work if that is something that should be added.

[marcelwgn]

Yes, you can set aria attributes as properties. I've updated my PR #855 to also expose the id property now. I agree that aria is something that is out of scope for this issue and should be tracked if the need arises.

[nmetulev]

Sounds good - let me know when you've updated the PR with the id property and I'll merge it :)

[marcelwgn]

Oh forgot to push the commit haha, commit is in the PR now, only waiting for CI to finish now.

[marcelwgn]

@nmetulev CI for #855 passed with the id changes now.