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

Migrate apikeys to React #6390

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Migrate apikeys to React #6390

wants to merge 13 commits into from

Conversation

viown
Copy link
Member

@viown viown commented Dec 16, 2024

Changes

  • Migrates the dashboard api keys page to React + TS SDK
  • Add an onClick listener to ButtonElement similar to IconButtonElement

Issues

@viown viown requested a review from a team as a code owner December 16, 2024 11:56
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Dec 16, 2024

Cloudflare Pages deployment

Latest commit 01d3538
Status 🔄 Deploying...
Preview URL Not available
Type 🔀 Preview

Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

Can we also migrate the page to using MUI components?

src/apps/dashboard/routes/_asyncRoutes.ts Outdated Show resolved Hide resolved
@viown
Copy link
Member Author

viown commented Jan 7, 2025

Updated look with mui:
image

@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jan 22, 2025
@thornbill thornbill added the typescript PRs or issues relating to TypeScript code label Jan 22, 2025
Copy link
Member

Choose a reason for hiding this comment

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

It seems like these changes are no longer necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct -- but it shouldn't hurt. Especially if we plan to use this component anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer we wait until/if it is needed.

Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

The table looks and functions well in my testing! Just a couple minor ui suggestions.

},

// Enable (delete) row actions
enableRowActions: true,
Copy link
Member

Choose a reason for hiding this comment

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

Let's position the action column at the end of the row:

Suggested change
enableRowActions: true,
enableRowActions: true,
positionActionsColumn: 'last',

},

renderTopToolbarCustomActions: () => (
<Button onClick={showNewKeyPopup}>{globalize.translate('HeaderNewApiKey')}</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a "+" icon to the button?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleanup of legacy code or code smells typescript PRs or issues relating to TypeScript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants