Skip to content

Conversation

@vikasgurjar
Copy link
Contributor

@vikasgurjar vikasgurjar commented Dec 5, 2024

Closes #4406

  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature.
  • I've linked relevant GitHub issue with "Closes #".
  • I've added a visual demonstration in the form of a screenshot or video.

@Janpot
Copy link
Member

Janpot commented Dec 5, 2024

I'm wondering if we should maintain the convention of naming a controlled prop "xyz" + "onXyzChange".

edit: actually, it looks like the data grid has the convention of "xyzOpen", "onXyzOpen" and "onXyzClose". Perhaps that's the closest in spirit?

@vikasgurjar
Copy link
Contributor Author

vikasgurjar commented Dec 6, 2024

edit: actually, it looks like the data grid has the convention of "xyzOpen", "onXyzOpen" and "onXyzClose". Perhaps that's the closest in spirit?

@Janpot Are you suggesting to have two different callbacks for open and close events? onSidebarOpen and onSidebarClosed?

Also can you help me with pointing out why some of the checks are failing?

@bharatkashyap
Copy link
Collaborator

bharatkashyap commented Dec 6, 2024

Also can you help me with pointing out why some of the checks are failing?

  • When changing any of the demos and prop types, make sure to run pnpm docs:typescript:formatted to format TS demos and pnpm:proptypes to autogenerate prop type changes
  • The ml change from 1 to 0.5 is causing a Argos to complain about the visual diff, not really sure if that margin reduction is required though, is it?
  • Also noticed the removal of the branding prop usage, and the alignItems: center - again probably not needed

@vikasgurjar
Copy link
Contributor Author

vikasgurjar commented Dec 6, 2024

  • When changing any of the demos and prop types, make sure to run pnpm docs:typescript:formatted to format TS demos and pnpm:proptypes to autogenerate prop type changes

I ran that command before raising the PR. What I have noticed is this lines props are wrongly generated. Correct proptype should be Proptype.func.required I think.

  • The ml change from 1 to 0.5 is causing a Argos to complain about the visual diff, not really sure if that margin reduction is required though, is it?

I did not make any such changes

@bharatkashyap
Copy link
Collaborator

bharatkashyap commented Dec 6, 2024

I did not make any such changes

I'm talking about L131, L377 and L386

What I have noticed is this lines props are wrongly generated. Correct proptype should be Proptype.func.required I think.

I'll try and look into it

@vikasgurjar
Copy link
Contributor Author

I will just raise another PR, made some mistakes while rebasing last time

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.

Add props to control sidebar's collapsed state in DashboardLayout component

3 participants