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

refactor(client): Use subcomponents for InfoPanel and UserInfo #32470

Merged
merged 15 commits into from
Jul 9, 2024

Conversation

rique223
Copy link
Contributor

@rique223 rique223 commented May 22, 2024

Proposed changes (including videos or screenshots)

Refactored all places that use InfoPanel and UserInfo components as objects and implemented subcomponents and barrel exports/imports for both of them.

Jira task: CONN-268

Issue(s)

Steps to test or reproduce

Further comments

Copy link
Contributor

dionisio-bot bot commented May 22, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 6.11.0, but it targets 6.10.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 22, 2024

⚠️ No Changeset found

Latest commit: a96c323

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@rique223 rique223 changed the base branch from develop to feat/new-user-panel-pending May 22, 2024 20:33
Base automatically changed from feat/new-user-panel-pending to develop June 21, 2024 02:54
Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

Project coverage is 56.89%. Comparing base (ca3d90b) to head (a96c323).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32470      +/-   ##
===========================================
+ Coverage    56.82%   56.89%   +0.06%     
===========================================
  Files         2497     2498       +1     
  Lines        55304    55345      +41     
  Branches     11405    11415      +10     
===========================================
+ Hits         31428    31490      +62     
+ Misses       21170    21152      -18     
+ Partials      2706     2703       -3     
Flag Coverage Δ
e2e 56.53% <42.85%> (+0.06%) ⬆️
unit 72.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@rique223 rique223 marked this pull request as ready for review June 21, 2024 19:09
@rique223 rique223 requested a review from a team as a code owner June 21, 2024 19:09
Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

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

Do you mind changing this notation to all places that uses InfoPanel subcomponents, so we can remove the exported object from apps/meteor/client/components/InfoPanel/index.ts

also prefer using import { subComponents } from '../InfoPanel/'

Btw, why so many commits?

@rique223
Copy link
Contributor Author

Do you mind changing this notation to all places that uses InfoPanel subcomponents, so we can remove the exported object from apps/meteor/client/components/InfoPanel/index.ts

also prefer using import { subComponents } from '../InfoPanel/'

Btw, why so many commits?

Sure, will do. About the commits I am not entirely sure, this branch was checked out of the feat/new-user-panel-pending branch that was recently merged and when it happened git automatically rebased this branch to the develop and duplicated a lot of the commits that were merged in the other branch. I assumed that since the git diff is correct it wouldn't be a problem, but if there's anyway to remove those commits I'd like do know, I'll do a little research on this as well.

@rique223 rique223 changed the base branch from develop to 0.20 June 21, 2024 19:34
@rique223 rique223 changed the base branch from 0.20 to develop June 21, 2024 19:34
tassoevan
tassoevan previously approved these changes Jun 21, 2024
@tassoevan tassoevan dismissed their stale review June 21, 2024 19:45

There is some odd thing going on with the commits that should be addressed first.

@rique223 rique223 changed the base branch from develop to release-6.8.1 June 21, 2024 19:53
@rique223 rique223 requested review from a team as code owners June 21, 2024 19:53
@rique223 rique223 changed the base branch from release-6.8.1 to develop June 21, 2024 19:53
@tassoevan tassoevan force-pushed the chore/contextual-bar-subcomponents branch 2 times, most recently from 67fdada to 0ca116c Compare June 21, 2024 20:48
@tassoevan tassoevan force-pushed the chore/contextual-bar-subcomponents branch from 0ca116c to 4de9d31 Compare June 21, 2024 20:50
@tassoevan tassoevan changed the title chore: Change UserInfo.tsx to subcomponents refactor(client): Use InfoPanel subcomponents in UserInfo Jun 21, 2024
@rique223 rique223 force-pushed the chore/contextual-bar-subcomponents branch from 32a338a to 4587152 Compare June 21, 2024 21:31
@rique223
Copy link
Contributor Author

rique223 commented Jun 21, 2024

Do you mind changing this notation to all places that uses InfoPanel subcomponents, so we can remove the exported object from apps/meteor/client/components/InfoPanel/index.ts
also prefer using import { subComponents } from '../InfoPanel/'
Btw, why so many commits?

Sure, will do. About the commits I am not entirely sure, this branch was checked out of the feat/new-user-panel-pending branch that was recently merged and when it happened git automatically rebased this branch to the develop and duplicated a lot of the commits that were merged in the other branch. I assumed that since the git diff is correct it wouldn't be a problem, but if there's anyway to remove those commits I'd like do know, I'll do a little research on this as well.

For Future Reference:

The problem Doug mentioned was caused by squash merging the feat/new-user-panel-pending branch. When a branch is squash merged, all the commits in the pull request (PR) are combined into a single commit on the base branch. Because of this, Git loses the ability to track the individual commits from the original branch. If a child branch was created from the feat/new-user-panel-pending branch before the squash merge, it still contains those individual commits. As a result, when the child branch is compared or a PR is created from it, Git will not recognize that those commits were already merged, leading to duplicate commit history in the PR just like happened here;

@rique223 rique223 removed stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Jun 25, 2024
@tassoevan tassoevan added the stat: QA assured Means it has been tested and approved by a company insider label Jun 25, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 25, 2024
@rique223 rique223 removed the request for review from a team June 26, 2024 18:38
@rique223 rique223 removed the request for review from dougfabris June 26, 2024 20:07
@tassoevan tassoevan requested review from dougfabris and removed request for dougfabris July 5, 2024 18:03
@ggazzo ggazzo merged commit b553cfc into develop Jul 9, 2024
48 checks passed
@ggazzo ggazzo deleted the chore/contextual-bar-subcomponents branch July 9, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants