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

Update profileCard.tsx #942

Closed
wants to merge 4 commits into from
Closed

Update profileCard.tsx #942

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1542e1
Update profileCard.tsx
GradleD Nov 10, 2024
671e906
updated loading
GradleD Nov 10, 2024
85eb8bb
pass the isLoading prop to all instances
GradleD Nov 11, 2024
e6d3daa
Some Improvements on component unmount.
GradleD Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion components/UI/profileCard/profileCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import Typography from "../typography/typography";
import { calculateTotalBalance } from "../../../services/argentPortfolioService";
import Avatar from "../avatar";
import { useHidePortfolio } from "@hooks/useHidePortfolio";
import Loading from "@components/skeletons/loading";

const MAX_RETRIES = 1000;
const RETRY_DELAY = 2000;
Expand All @@ -36,6 +37,7 @@ type ProfileCardProps = {
identity: Identity;
leaderboardData: LeaderboardToppersData;
isOwner: boolean;
isLoading:boolean;
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing isLoading prop to component parameters

The isLoading prop is defined in ProfileCardProps but not included in the component's parameter destructuring. This makes the prop inaccessible within the component.

Apply this fix:

const ProfileCard: FunctionComponent<ProfileCardProps> = ({
  rankingData,
  identity,
  leaderboardData,
  isOwner,
+ isLoading,
}) => {

Also applies to: 44-48

Copy link
Contributor

Choose a reason for hiding this comment

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

You add is Loading to the component props but you don't add it where the component is used + you unforce it.

Please test the stuff you send before a PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@fricoben, thank you for pointing this out. You're absolutely correct; the isLoading prop was added to the ProfileCard component but wasn't included where the component is used. To fix this, please ensure that isLoading is passed as a prop in all instances where ProfileCard is instantiated.

For example:

<ProfileCard
  rankingData={rankingData}
  identity={identity}
  leaderboardData={leaderboardData}
  isOwner={isOwner}
  isLoading={isLoading}
/>

Additionally, it's important to thoroughly test your changes before submitting a PR to avoid such oversights.

};

const ProfileCard: FunctionComponent<ProfileCardProps> = ({
Expand Down Expand Up @@ -122,6 +124,21 @@ const ProfileCard: FunctionComponent<ProfileCardProps> = ({
</div>

<div className="flex flex-col h-full justify-center">
{totalBalance === null ? ( // Check if balance is still loading
<>
//Updated the Specific Profile Card Component with skeleton Loading component
<Skeleton variant="text" width="80%" height={20} />
<Skeleton variant="text" width="60%" height={30} className="mt-2" />
<div className={styles.address_div}>
<div className="flex items-center gap-2">
<Skeleton variant="rectangular" width={100} height={30} />
<EyeIcon />
</div>
</div>
</>
) : (
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve loading state implementation

Several issues to address in the loading state implementation:

  1. Using totalBalance === null as a loading indicator is implicit. Consider using the explicit isLoading prop instead.
  2. The comment is incorrectly formatted and should be wrapped in braces.
  3. The skeleton UI could be more semantic with aria labels for accessibility.

Apply these improvements:

- {totalBalance === null ? ( // Check if balance is still loading
+ {isLoading ? (
  <>
-   //Updated the Specific Profile Card Component with skeleton Loading component
+   {/* Profile information loading state */}
    <Skeleton 
+     aria-label="Loading profile name"
      variant="text" 
      width="80%" 
      height={20} 
    />
    <Skeleton 
+     aria-label="Loading balance"
      variant="text" 
      width="60%" 
      height={30} 
      className="mt-2" 
    />
    <div className={styles.address_div}>
      <div className="flex items-center gap-2">
        <Skeleton 
+         aria-label="Loading address"
          variant="rectangular" 
          width={100} 
          height={30} 
        />
        <EyeIcon />
      </div>
    </div>
  </>

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome

[error] 129-129: Wrap comments inside children within braces.

Unsafe fix: Wrap the comments with braces

(lint/suspicious/noCommentText)

<>

<Typography
type={TEXT_TYPE.BODY_SMALL}
color="secondary"
Expand Down Expand Up @@ -151,7 +168,8 @@ const ProfileCard: FunctionComponent<ProfileCardProps> = ({
{hidePortfolio ? <EyeIconSlashed /> : <EyeIcon />}
</div>
</div>
</div>
</>
)}
<div className="flex sm:hidden justify-center py-4">
<SocialMediaActions identity={identity} />
{tweetShareLink && (
Expand Down