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

Implement Navidrome sharing #558

Closed
wants to merge 6 commits into from
Closed

Implement Navidrome sharing #558

wants to merge 6 commits into from

Conversation

iiPythonx
Copy link
Contributor

@iiPythonx iiPythonx commented Mar 23, 2024

PR that adds an option for sharing an album or songs via Navidrome (nothing else yet), resolves #80.
There are more then likely a lot of issues with how I did this, if you have any feedback on it please lmk (considering I've still barely touched React in my life).

Two nitpicks:

  • Right now, this PR selects resourceType based on the first item you've selected. This should be fine since I'm assuming you can't magically select two items of different types within Feishin (ie. selecting an album and a song at the same time).
  • Additionally, there seems to be a lack of margin on TextInput fields (not that bad but it's a bit squished):

image

Anyways, as stated in the beginning, any feedback would be very much appreciated.

Copy link

vercel bot commented Mar 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
feishin ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 1, 2024 3:35am

@iiPythonx iiPythonx changed the title Implement Navidrome sharing (#80) Implement Navidrome sharing Mar 23, 2024
Copy link
Collaborator

@kgarner7 kgarner7 left a comment

Choose a reason for hiding this comment

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

Sharing is not available in every Navidrome. Make sure to add it as a feature in feature-types.ts (ServerFeature), modify navidrome-controllers.ts#getServerInfo to check for the correct version, and check if that feature exists.

src/renderer/api/controller.ts Show resolved Hide resolved
src/renderer/api/navidrome/navidrome-types.ts Show resolved Hide resolved
src/renderer/api/types.ts Show resolved Hide resolved
Copy link
Collaborator

@kgarner7 kgarner7 left a comment

Choose a reason for hiding this comment

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

I added a few changes. It might be nice to have the ability to view existing shares, but this is a lower priority (I may take a look at implementing this).

I'm not particularly a fan of having the share only copied to clipboard, as that might not work especially in browser environments where clipboard access is limited. It would be good to have a way to open in browser (maybe when clicking on the notification?)

@iiPythonx
Copy link
Contributor Author

It would be good to have a way to open in browser (maybe when clicking on the notification?)

Went ahead and made it so clicking on the success notification does indeed open the share in browser.

It might be nice to have the ability to view existing shares, but this is a lower priority (I may take a look at implementing this).

I'll leave that up to you then (if you decide to work on it).

Copy link
Collaborator

@kgarner7 kgarner7 left a comment

Choose a reason for hiding this comment

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

...ngl, adding share listing is more of a pain than I'd like. It can come as a feature later.

@kgarner7 kgarner7 requested a review from jeffvli March 30, 2024 18:50
@iiPythonx iiPythonx closed this by deleting the head repository Apr 5, 2024
@iiPythonx
Copy link
Contributor Author

Replacement is at #575 (sorry!).

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.

Share from context menu
2 participants