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

Add Cache and Server Cards to Map #1947

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

CannonLock
Copy link
Contributor

@CannonLock CannonLock commented Jan 27, 2025

  • Add more data onClick to the map server components

Redacted to prevent scope creep (This is ready, but I would like to tack on a couple items before merge and need the director data to work first)

#1778

- Add more data onClick to the map server components
@CannonLock CannonLock marked this pull request as draft January 27, 2025 20:57
@CannonLock CannonLock marked this pull request as ready for review January 30, 2025 18:14
@CannonLock CannonLock linked an issue Feb 4, 2025 that may be closed by this pull request
@CannonLock CannonLock requested a review from h2zh February 7, 2025 20:52
@CannonLock CannonLock added the enhancement New feature or request label Feb 10, 2025
@CannonLock CannonLock added this to the v7.14 milestone Feb 10, 2025
Copy link
Collaborator

@h2zh h2zh left a comment

Choose a reason for hiding this comment

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

Looks good in general, few things could be done to further improve!

<InformationSpan name={'URL'} value={server.url} />
<InformationSpan
name={'Longitude'}
value={server.longitude.toString()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I vaguely remember from my React class that in JSX, React can automatically convert numbers to strings when rendering them. So toString() here and the one on line 30 can be waived.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does but I have the InformationSpan typed like so.

export const InformationSpan = ({
  name,
  value,
  indent = 0,
}: {
  name: string;
  value: string;
  indent?: number;
}) => {

So I just cast it to pass the type check. To be fair I could accept quite a few more values and have more lenient typing I suppose.

Copy link
Collaborator

@h2zh h2zh left a comment

Choose a reason for hiding this comment

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

🚀

@CannonLock CannonLock merged commit bc1c737 into PelicanPlatform:main Feb 18, 2025
22 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map at /view/director/map/ could benefit from extra labels
2 participants