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

Adds avatarUrl handling to the hidden refs popover #3936

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nzaytsev
Copy link
Contributor

@nzaytsev nzaytsev commented Jan 16, 2025

Description

Tried to reuse methods and components from the GraphComponents module, but it cannot be imported. Implemented very simplified copies of required functions to show avatarUrl and sort the list

Screenshot 2025-01-16 at 11 50 54
Screenshot 2025-01-16 at 11 51 07

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@nzaytsev nzaytsev force-pushed the bugs/3931-show-remote-avatar-in-hidden-refs-popover branch from d0564d9 to 34a2e4f Compare January 16, 2025 04:20
@nzaytsev nzaytsev force-pushed the bugs/3931-show-remote-avatar-in-hidden-refs-popover branch from 34a2e4f to c6a5767 Compare January 16, 2025 04:49
import React from 'react';
import { CodeIcon } from '../../../shared/components/code-icon.react';

// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for disabling lint here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our eslint config works incorrect with react component names because react component name is not camelCased

import { CodeIcon } from '../../../shared/components/code-icon.react';

// eslint-disable-next-line @typescript-eslint/naming-convention
export function RemoteIcon({ refOptData }: Readonly<{ refOptData: GraphRefOptData }>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file feels more like a subcomponent, not a util.


// eslint-disable-next-line @typescript-eslint/naming-convention
export function RemoteIcon({ refOptData }: Readonly<{ refOptData: GraphRefOptData }>) {
console.log({ ref: refOptData });
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove console logs before moving to PR.

Comment on lines +1 to +2
import type { GraphRefOptData } from '@gitkraken/gitkraken-components';
import { refTypes } from '@gitkraken/gitkraken-components';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that the component is bundled such that importing from it in more files won't affect our bundle size, so this may or may not matter, but let's try to keep these imports in protocol of this domain and then pass them here, like we did with other types.

Comment on lines +4 to +13
export function compareGraphRefOpts(a: GraphRefOptData, b: GraphRefOptData): number {
const comparationResult = a.name.localeCompare(b.name);
if (comparationResult === 0) {
// If names are equals
if (a.type === refTypes.REMOTE) {
return -1;
}
}
return comparationResult;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use a comment explaining what is being compared and why. Also, for my own understanding, why are we only considering the type of a and not b?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is copied from GitkrakenComponents lib

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.

Graph hidden branches/tags dropdown: show remote avatar for remote branches if configured
2 participants