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 last used collection to right click menu. #7491

Merged
merged 4 commits into from
Mar 8, 2022

Conversation

Ruk33
Copy link
Collaborator

@Ruk33 Ruk33 commented Feb 22, 2022

Fixes

Issue Number: #6205

What is the current behavior?

The last used collection is not displayed in the menu options.

What is the new behavior?

The last used collection is displayed in the menu options:

image
image

Other information

PR Checklist

Toggle...

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I added a line describing my change to CHANGELOG.md
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

@jessopb
Copy link
Member

jessopb commented Feb 23, 2022

Nice.
One thing - I found that after publishing a list, adding a claim to it does not update the recent item.

@Ruk33
Copy link
Collaborator Author

Ruk33 commented Feb 24, 2022

Thank you @jessopb I have sent a few changes for that.

@jessopb
Copy link
Member

jessopb commented Feb 27, 2022

Hm, Can you just switch the recent claim in [ACTIONS.COLLECTION_PENDING]: (state, action) => { ?

@Ruk33
Copy link
Collaborator Author

Ruk33 commented Mar 4, 2022

@jessopb I just tested updating the last used collection by only using ACTIONS.COLLECTION_PENDING but it doesn't work as expected. Please let me know if this is what you meant by your comment.
Thanks!

@jessopb
Copy link
Member

jessopb commented Mar 4, 2022

If lastUsed is just the uuid localId of the local collection, or the claimId of the published collection, then you can use a selector
collection: makeSelectCollectionForId(props.collectionId)(state), whenever you need the collection.
The only time that lastUsed needs to change from localId to claimId is when COLLECTION_PENDING happens.
There may be an edge case about deleting or unpublishing.

Does this make sense, or am I missing something? What part wasn't working?

@Ruk33
Copy link
Collaborator Author

Ruk33 commented Mar 8, 2022

Thank you @jessopb you were right, it's much cleaner now!

@jessopb jessopb self-requested a review March 8, 2022 17:30
@lbry-bot lbry-bot assigned jessopb and unassigned jessopb Mar 8, 2022
if (lastUsedCollection) {
newLastUsedCollection = newAllCollections.find((collection) => {
// $FlowFixMe
return collection.name === lastUsedCollection.name;
Copy link
Member

Choose a reason for hiding this comment

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

We don't have a guarantee that collection.name is unique.

newPendingList[claimId] = Object.assign({}, newUnpublishedList[localId] || {});

Possibly here, we need to carry the old localId:

      if (localId) {
        // new publish
        newPendingList[claimId] = Object.assign({}, newUnpublishedList[localId] || {});
        newPendingList[claimId]['previousId'] = localId; // <--------
        delete newUnpublishedList[localId]; 
      } else {
      

Or maybe the collections reducer can keep an object mapping claimIds to localId that can just be looked up. The entry would be deleted after that pending claim is confirmed.

@jessopb jessopb merged commit 66bdd3f into master Mar 8, 2022
@jessopb jessopb deleted the 6205-remember-last-used-playlist-choice branch March 8, 2022 17:30
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.

2 participants