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.

Sign up for GitHub

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

Jump to bottom

[FIX] Misaligned username on Room Info card for omnichannel chats #25331

Merged
merged 9 commits into from
Jun 30, 2022
Merged

[FIX] Misaligned username on Room Info card for omnichannel chats #25331

merged 9 commits into from
Jun 30, 2022

Conversation

murtaza98
Copy link
Contributor

Proposed changes (including videos or screenshots)

Issue(s)

image

Steps to test or reproduce

Further comments

@murtaza98 murtaza98 requested a review from dougfabris April 28, 2022 05:41
@murtaza98 murtaza98 mentioned this pull request Apr 28, 2022
Merged
@murtaza98 murtaza98 added this to the 4.7.0 milestone Apr 28, 2022
dougfabris
dougfabris previously requested changes Apr 28, 2022
View reviewed changes
Copy link
Member

@dougfabris dougfabris left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@murtaza98 I strongly recommend you replace UserCard.Username where Omnichannel is using it incorrectly:

  • apps/meteor/client/views/omnichannel/directory/calls/contextualBar/VoipInfo.tsx
  • apps/meteor/client/views/omnichannel/directory/chats/contextualBar/AgentField.js
  • apps/meteor/client/views/omnichannel/directory/chats/contextualBar/ContactField.js
  • apps/meteor/client/views/omnichannel/directory/contacts/contextualBar/ContactInfo.js
  • apps/meteor/ee/client/omnichannel/ContactManagerInfo.js

@murtaza98
Copy link
Contributor Author

Hey, @dougfabris Thanks for the review!! I've made the changes you requested. Do you mind taking a look at it again?

@murtaza98 murtaza98 requested a review from dougfabris April 29, 2022 05:26
@murtaza98 murtaza98 dismissed dougfabris’s stale review April 29, 2022 05:27

Requested changes added

dougfabris
dougfabris previously requested changes Apr 29, 2022
View reviewed changes
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.

@murtaza98 UserCard.Username should be used only inside UserCard 😢

@murtaza98 murtaza98 modified the milestones: 4.7.0, 4.8.0 May 2, 2022
@murtaza98 murtaza98 changed the title Regression: Misaligned username on Room Info card for omnichannel chats Fix: Misaligned username on Room Info card for omnichannel chats May 2, 2022
ggazzo
ggazzo previously requested changes May 2, 2022
View reviewed changes
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

UserCard.Username is a component developed and meant to be used inside UserCard

this usage is wrong and there is no guarantee that tomorrow we wont change/remove or do whatever we want without consider outside usages

@ggazzo
Copy link
Member

ggazzo commented May 2, 2022

[FIX] ;)

@KevLehman KevLehman changed the title Fix: Misaligned username on Room Info card for omnichannel chats [FIX] Misaligned username on Room Info card for omnichannel chats May 2, 2022
@murtaza98 murtaza98 force-pushed the reg-omni-username-misalign-room branch from 6022fe0 to 64003cf Compare May 9, 2022 08:29
@murtaza98 murtaza98 requested a review from a team as a code owner May 9, 2022 08:29
@murtaza98
Copy link
Contributor Author

Thanks for the review guys. I've now removed the usage of UserCard.Username component from all the omnichannel components & created an instead created a similar new component for omnichannel usage. Please review it again when you get a chance. Thanks!

@murtaza98 murtaza98 dismissed stale reviews from ggazzo and dougfabris May 9, 2022 08:35

Changes done :)

@murtaza98 murtaza98 force-pushed the reg-omni-username-misalign-room branch from 64003cf to b5e4dd9 Compare May 10, 2022 06:49
dougfabris
dougfabris previously requested changes May 10, 2022
View reviewed changes
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.

I still missing the replacement on this file: apps/meteor/client/views/omnichannel/directory/calls/contextualBar/VoipInfo.tsx

apps/meteor/client/components/Omnichannel/AgentOrContactCardUsername.tsx Outdated Show resolved Hide resolved
@murtaza98 murtaza98 dismissed dougfabris’s stale review May 18, 2022 15:04

Requested changes done. Thanks Douglas :)

@d-gubert d-gubert modified the milestones: 4.8.0, 4.9.0 May 24, 2022
@murtaza98 murtaza98 changed the title [FIX] Misaligned username on Room Info card for omnichannel chats Regression: Misaligned username on Room Info card for omnichannel chats May 25, 2022
@murtaza98 murtaza98 modified the milestones: 5.0.0, 4.8.0 May 25, 2022
ggazzo
ggazzo previously requested changes May 25, 2022
View reviewed changes
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

There is no card at this component, the component name is weird

@murtaza98
Copy link
Contributor Author

There is no card at this component, the component name is weird

Yeah, I agree since this isn't a card. I have removed it now. Thanks :)

@KevLehman KevLehman modified the milestones: 4.8.0, 5.0.0 May 26, 2022
@KevLehman KevLehman changed the title Regression: Misaligned username on Room Info card for omnichannel chats [FIX] Misaligned username on Room Info card for omnichannel chats May 26, 2022
@KevLehman
Copy link
Contributor

So this was a fix after all 😂

@murtaza98 murtaza98 requested a review from ggazzo May 30, 2022 13:14
@murtaza98 murtaza98 dismissed ggazzo’s stale review June 7, 2022 06:51

Changes done! Thanks @gaZZo

ggazzo
ggazzo previously requested changes Jun 27, 2022
View reviewed changes
apps/meteor/ee/client/omnichannel/ContactManagerInfo.js Outdated Show resolved Hide resolved
@murtaza98 murtaza98 requested a review from ggazzo June 28, 2022 13:54
@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jun 29, 2022
@murtaza98 murtaza98 dismissed ggazzo’s stale review June 29, 2022 08:24

Changes done

@kodiakhq kodiakhq bot merged commit 6c0864f into develop Jun 30, 2022
@kodiakhq kodiakhq bot deleted the reg-omni-username-misalign-room branch June 30, 2022 04:43
@murtaza98 murtaza98 mentioned this pull request Jul 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevLehman KevLehman KevLehman approved these changes

@ggazzo ggazzo ggazzo approved these changes

@MartinSchoeler MartinSchoeler Awaiting requested review from MartinSchoeler

@tiagoevanp tiagoevanp Awaiting requested review from tiagoevanp

@dougfabris dougfabris Awaiting requested review from dougfabris

Assignees
No one assigned
Labels
stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@murtaza98 @ggazzo @KevLehman @dougfabris @d-gubert