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

Navigate to collection from movie page #9399

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

Axiloom
Copy link
Contributor

@Axiloom Axiloom commented Nov 13, 2023

Database Migration

NO

Description

Adds the ability to click the collection name on a specific movie which will take the user to the specific collection on the collections page

Screenshot (if UI related)

Screen Shot 2023-11-12 at 6 28 12 PM

Todos

  • Tests
  • Translation Keys (./src/NzbDrone.Core/Localization/Core/en.json)
  • Wiki Updates

Issues Fixed or Closed by this PR

@github-actions github-actions bot added the Area: UI Issue is related to UI, should also add the issue to the new UI project, if it isn't a bug. label Nov 13, 2023
Copy link
Collaborator

@mynameisbogdan mynameisbogdan left a comment

Choose a reason for hiding this comment

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

I think we could use another color for the link though.

Otherwise sounds awesome, looking forward to try it.

@@ -9,7 +9,7 @@ import { Link as RouterLink } from 'react-router-dom';
import styles from './Link.css';

interface ReactRouterLinkProps {
to?: string;
to?: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs an undo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a bit of a rework on this component that I'll commit up soon. Turns out how we are using this intermittent Link makes the type of to somewhat challenging.

frontend/src/Movie/MovieCollectionLabel.js Outdated Show resolved Hide resolved
@Axiloom
Copy link
Contributor Author

Axiloom commented Nov 18, 2023

@mynameisbogdan any particular color you suggest for the link?

@mynameisbogdan
Copy link
Collaborator

any particular color you suggest for the link?

To be honest, I think keeping it white as the rest of the content on that line would be also nice.

@Axiloom
Copy link
Contributor Author

Axiloom commented Nov 26, 2023

any particular color you suggest for the link?

To be honest, I think keeping it white as the rest of the content on that line would be also nice.

It should now be inheriting its color to be the same as the other labels on the same page.

@Axiloom
Copy link
Contributor Author

Axiloom commented Dec 2, 2023

Any update on this?

@Qstick
Copy link
Member

Qstick commented Dec 31, 2023

@mynameisbogdan I'm good with this if you are

@mynameisbogdan
Copy link
Collaborator

mynameisbogdan commented Dec 31, 2023

AFAIK it's not working, since id for collection is 0 for all movies at the moment.

var collection = model.MovieMetadata.Value.CollectionTmdbId > 0 ? new MovieCollection { Title = model.MovieMetadata.Value.CollectionTitle, TmdbId = model.MovieMetadata.Value.CollectionTmdbId } : null;

@Qstick
Copy link
Member

Qstick commented Dec 31, 2023

Ah, hence the other issue? For this purpose we can use collection selector with the collection tmdbid from the movie object. All the collections are loaded into the UI at page load.

@mynameisbogdan
Copy link
Collaborator

Yeah, I was thinking about tmdbid as well to avoid pulling collection data into the resource mapper.

@Qstick
Copy link
Member

Qstick commented Dec 31, 2023

Yea, we should do that. That's what the monitor toggle already does.

@Axiloom
Copy link
Contributor Author

Axiloom commented Jan 19, 2024

@mynameisbogdan Ok I think I did this correctly, but please let me know if I missed something. Thanks for reviewing this

@mynameisbogdan
Copy link
Collaborator

mynameisbogdan commented Jan 21, 2024

Hello, just tried it locally and the link works. But if you scroll on the collections page and refresh the page, it's sending you back to the initial collection. Same if you try to jump to another letter by clicking multiple times.

@@ -53,7 +53,8 @@ class CollectionOverviews extends Component {
columnCount: 1,
posterWidth: 162,
posterHeight: 238,
rowHeight: calculateRowHeight(238, null, props.isSmallScreen, {})
rowHeight: calculateRowHeight(238, null, props.isSmallScreen, {}),
navigateToId: props.location.state ? props.location.state.navigateToId : 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
navigateToId: props.location.state ? props.location.state.navigateToId : 0
navigateToId: props.location.state?.navigateToId ?? 0

@@ -40,7 +50,8 @@ class MovieCollectionLabel extends Component {
MovieCollectionLabel.propTypes = {
title: PropTypes.string.isRequired,
monitored: PropTypes.bool.isRequired,
onMonitorTogglePress: PropTypes.func.isRequired
onMonitorTogglePress: PropTypes.func.isRequired,
tmdbId: PropTypes.string.isRequired
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tmdbId: PropTypes.string.isRequired
tmdbId: PropTypes.number.isRequired

@Axiloom
Copy link
Contributor Author

Axiloom commented Feb 3, 2024

@mynameisbogdan Should be all fixed and I applied your code suggestions

@Axiloom
Copy link
Contributor Author

Axiloom commented Feb 3, 2024

Forgot one little thing, should be good after my last commit

@Axiloom
Copy link
Contributor Author

Axiloom commented Feb 17, 2024

Any update on this? Are we able to get this into a release?

@mynameisbogdan
Copy link
Collaborator

We should be able, but I want to try the changes again which I didn't had the time to do so yet.

@GlassedSilver
Copy link

Any update? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI Issue is related to UI, should also add the issue to the new UI project, if it isn't a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clickable Collection Name (Movie Page)
4 participants