Skip to content

Conversation

@cs25-esc
Copy link
Contributor

@cs25-esc cs25-esc commented Nov 1, 2023

This fix resolves:

The avatars will not expand if alignRight prop is True and disableExpand prop is True.
Every combination works as expected.

Screen.Recording.2023-11-01.at.1.53.39.AM.mov

@cs25-esc cs25-esc requested review from a team and broccolinisoup November 1, 2023 10:47
@changeset-bot
Copy link

changeset-bot bot commented Nov 1, 2023

🦋 Changeset detected

Latest commit: 0b7a401

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cs25-esc cs25-esc temporarily deployed to github-pages November 1, 2023 10:54 — with GitHub Actions Inactive
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Hi @cs25-esc 👋🏻 Thanks so much for pushing this PR! It looks good to me. Could you please add a changeset as well? https://github.com/primer/react/blob/main/contributor-docs/CONTRIBUTING.md#adding-changeset-to-your-pull-request Thank you!

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 2, 2023

Hi @maximedegreve / @siddharthkp / @lesliecdubs could you help me with this?

@cs25-esc cs25-esc temporarily deployed to github-pages November 2, 2023 04:29 — with GitHub Actions Inactive
@cs25-esc cs25-esc mentioned this pull request Nov 2, 2023
11 tasks
@siddharthkp
Copy link
Member

siddharthkp commented Nov 2, 2023

Added changeset 👍

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 2, 2023

Hi @siddharthkp is there any way to test CI / test (pull_request) locally ? i am suspecting the reason for failure is due to changes made in CSS class-level !

@siddharthkp
Copy link
Member

Hi @siddharthkp is there any way to test CI / test (pull_request) locally ? i am suspecting the reason for failure is due to changes made in CSS class-level !

Yes, if you have a local clone you can run npm run test and you should see the failing test. It looks like there's a snapshot that has changed. You can update that with npm run test:update. (see package.json for all our scripts)

Contributor docs: https://github.com/primer/react/blob/main/contributor-docs

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 3, 2023

Hi @siddharthkp done with the snapshot update !

Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Hello! 👋🏻 Thanks for following up with the changes. The only thing I would say is that it seems like package-lock changes in the diff includes things that are more than necessary updates. I pushed a PR to update the @primer/react version in the lock files #3904 and I was wondering if that would be possible to take the package-lock changes back in this PR? They are irrelevant to the AvatarStack fix as well. Let me know if you have an concerns or questions though! Thanks again for your contribution 🙏🏻

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 6, 2023

Hi @broccolinisoup i have made the necessary changes 👍 kindly review it please :)

@broccolinisoup
Copy link
Member

Hi @broccolinisoup i have made the necessary changes 👍 kindly review it please :)

Hi! Sorry maybe my comment wasn't clear. We updated the package-lock.json and package.json in another PR and it is merged now too so you don't need to push any versining changes. We should only see 3 files changed in the diff if you could please take the package.json and package-lock.json back? Thank you!

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 6, 2023

Hi @broccolinisoup i have made the necessary changes 👍 kindly review it please :)

Hi! Sorry maybe my comment wasn't clear. We updated the package-lock.json and package.json in another PR and it is merged now too so you don't need to push any versining changes. We should only see 3 files changed in the diff if you could please take the package.json and package-lock.json back? Thank you!

I have already made the changes package.json and package-lock.json back to its original files, sorry i am not getting what you mean

@broccolinisoup
Copy link
Member

broccolinisoup commented Nov 6, 2023

I have already made the changes package.json and package-lock.json back to its original files, sorry i am not getting what you mean

No worries! If you could view the latest @primer/react version in the package.json (reference), it is 36.1.0 but in this PR, the changes are taken back to 36.0.0

If you do git checkout main package.json package-lock.json in your branch and commit and push, I believe we won't see any package changes in the diff. Let me know if I clarify anything further 🙌🏻

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 6, 2023

I have already made the changes package.json and package-lock.json back to its original files, sorry i am not getting what you mean

No worries! If you could view the latest @primer/react version in the package.json (reference), it is 36.1.0 but in this PR, the changes are taken back to 36.0.0

If you do git checkout main package.json package-lock.json in your branch and commit and push, I believe we won't see any package changes in the diff. Let me know if I clarify anything further 🙌🏻

just did the needful, could you please verify now :)

@broccolinisoup
Copy link
Member

HI @cs25-esc 👋🏻 There are still 5 files in the diff and that includes package.json and package-lock.json. It is probably because of the local editor settings that caused prettier not being ran and remove the EOF lines.

Could you please run the following in your terminal?

npm run format package.json package-lock.json and make sure there is no package.json and package-lock.json in the diff?

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 7, 2023

hi @broccolinisoup please review now

Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

🥳

@cs25-esc
Copy link
Contributor Author

cs25-esc commented Nov 7, 2023

🥳

thanks @broccolinisoup and @siddharthkp this is my first open source contribution learnt a lot and lot. please let me know if anything still left from my end on this PR.

@siddharthkp
Copy link
Member

@broccolinisoup Feel free to merge when ready :)

@pksjce pksjce added this pull request to the merge queue Nov 7, 2023
Merged via the queue into primer:main with commit 9daea96 Nov 7, 2023
@primer primer bot mentioned this pull request Nov 7, 2023
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.

AvatarStack disableExpand and rightAlign don't work together

4 participants