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

added functionality to edit artist/title for tracks in a playlist #1737

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

Conversation

O1af
Copy link

@O1af O1af commented Nov 16, 2024

Description:
I have been working on this for the past few days and seem to have a working solution for #1735 The changes allow users to edit the artist or title field in a playlist by double-clicking it. When double-clicked, the field temporarily converts into an editable textbox, allowing users to make changes which are then saved.

Specifically, I have:

Added extra components for each field, which are wrappers on the existing titleCell and TextCell components. These components handle the conversion to editable textboxes and saving the changes.
Ensured that the implementation does not introduce significant complexity.
Request for Guidance:

Functionality seems to work when i manually test,however when running npm test i fail some of the snapshot tests(@nuclear/app:test). Was wondering if this is due to the additional functionality or if i did something wrong during development.

I would appreciate guidance on what tests to write for this new functionality. Specifically, I am looking for advice on:
The types of tests that would be most valuable (e.g., unit tests, integration tests).
Any edge cases or scenarios that I should be mindful of when writing these tests.

Please let me know if there are any additional details or changes needed before submission.

@nukeop
Copy link
Owner

nukeop commented Nov 16, 2024

Hey, since you've made some changes to the table, could you please update the snapshots everywhere? You can run npm test -- -u in the app directory, and commit the changes.

@O1af
Copy link
Author

O1af commented Nov 17, 2024

Done

@nukeop nukeop added the under review This pull request is being reviewed by maintainers. label Nov 18, 2024
@nukeop
Copy link
Owner

nukeop commented Nov 18, 2024

Hey, I reviewed your changes and I have the following feedback:

  • Looks like the filter button has changed its background to blue
before after
image image
  • Titles are now aligned to the top, they should be centered
image
  • This is relatively minor and can be easily handled later (or I can do that for you), but the inputs are unstyled:
image
  • While editing, there's no way to cancel or confirm from the keyboard. It would be nice to have Esc/Enter do that (as I see it currently saves changes only on blur)
  • It would be great to add some tests in packages/app/app/containers/PlaylistViewContainer/PlaylistViewContainer.test.tsx. You can use testing-library to double-click elements by name, and check if they turn into inputs, then simulate typing text and saving. This should be done for both kinds of editable fields. If you don't know where to start or how to do any of these steps, I'll be glad to help you.

All in all a very solid start and I believe that if we iterate on this together we can shape it into a great feature.

@O1af
Copy link
Author

O1af commented Nov 18, 2024

  1. Not sure why the filter is like that couldn't figure out as I didn't change any files/css that was remotely related to it
  2. fixed the alignment
  3. tried to add styles but didn't work
  4. I added esc/enter functionality
  5. added some tests but came across a bug and after 2hrs couldn't figure it out
    Keep getting TypeError: Cannot read properties of undefined (reading 'name') at PlaylistView (index.tsx:170:27)
    for all 20 tests not just the 4 I added and not sure why. Would appreciate some guidance on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
under review This pull request is being reviewed by maintainers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants