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

Add tooltip to organizations' identities in organizations list #1702

Closed
wants to merge 4 commits into from
Closed

Add tooltip to organizations' identities in organizations list #1702

wants to merge 4 commits into from

Conversation

peoray
Copy link
Contributor

@peoray peoray commented Oct 16, 2023 •
edited by ghost
Loading

Changes proposed ✍️

Fixes #1495

What

🤖 Generated by Copilot at fce6a06

Improved error handling, readability, and functionality of organization-identities.vue component. Added link support for platform badges.
​

🤖 Generated by Copilot at fce6a06

To prevent errors and make code more readable
They added some chaining operators that are optional
They also made PlatformBadge more versatile
With href and asLink props that are configurable
And refactored imports and getPlatformDetails to be less repeatable

Why

How

🤖 Generated by Copilot at fce6a06

  • Add optional chaining operators to prevent errors when accessing undefined properties (link, link)
  • Pass href prop and use asLink config to render platform badges as links or not (link, link)
  • Refactor getPlatformDetails function to return common platform badge properties and simplify props (link)
  • Reorder import statements to follow convention (link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

joanagmaia
joanagmaia requested changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

Just a small copy fix 😄

frontend/src/modules/organization/components/organization-identities.vue Outdated Show resolved Hide resolved
joanagmaia
joanagmaia approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

joanagmaia
joanagmaia requested changes Oct 24, 2023
View reviewed changes
frontend/src/modules/organization/components/organization-identities.vue
:track-event-name="getPlatformDetails(platform)?.trackEventName"
:track-event-channel="getPlatformDetails(platform)?.trackEventChannel"
:tooltip-label="getPlatformDetails(platform)?.tooltipLabel"
:show-handles-badge="true"
:as-link="getUrlsByPlatform(platform).length ? getUrlsByPlatform(platform)[0] : false"
:as-link="getPlatformDetails(platform).asLink"
Copy link
Contributor

Choose a reason for hiding this comment

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

I just double checked, and if organization identity does not have a url, we are redirecting to https://github.com/github. However, when there's no url in identity we should not show button as a link.
Could you fix this

@peoray
Copy link
Contributor Author

peoray commented Oct 26, 2023

@joanagmaia having issues running the FE

Screenshot 2023-10-26 at 10 57 39 PM

I have ran the DEV=1 ./cli service frontend up, as well as npm i inside the frontend directory but neither is fixing the issue

@joanagmaia
Copy link
Contributor

@joanagmaia having issues running the FE

Screenshot 2023-10-26 at 10 57 39 PM I have ran the `DEV=1 ./cli service frontend up`, as well as `npm i` inside the frontend directory but neither is fixing the issue

Hey @peoray sorry to hear about this!
Can you try to remove node_modules folder and install again from frontend directory? And start from there as well?

@peoray
Copy link
Contributor Author

peoray commented Oct 30, 2023

@joanagmaia that does not work, already done that multiple times including today.
Still getting the same error as the screenshot

@joanagmaia
Copy link
Contributor

Hey @peoray, we fixed this in the context of another task. Closing this PR as it is no longer required. Nonetheless thanks for your work here

@joanagmaia joanagmaia closed this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joanagmaia joanagmaia joanagmaia requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C-2314] Add tooltip to Organizations' identities in Organizations list
2 participants
@peoray @joanagmaia