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

Feature/episode order #154

Closed
wants to merge 7 commits into from
Closed

Conversation

XanderVertegaal
Copy link
Contributor

Closes #122

Adds a rank property to objects inheriting EntityDescription and allows the user to drag/drop episodes in a source's episode overview to reorder them (using Angular Material's CDK).

I'd be happy to discuss alternatives if you think rank-based ordering or a drag-and-drop interface are not the way to go.

@XanderVertegaal XanderVertegaal requested review from lukavdplas and removed request for lukavdplas October 25, 2024 10:47
@XanderVertegaal XanderVertegaal marked this pull request as draft October 28, 2024 13:38
@XanderVertegaal
Copy link
Contributor Author

This PR will be adjusted to replace the drag and drop interface with buttons to move episodes 'up'/'down' in the hierarchy.

Copy link
Contributor

@lukavdplas lukavdplas left a comment

Choose a reason for hiding this comment

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

For clarity, the point about the drag-and-drop UI was something we discussed outside of Github.

I added some additional comments on the backend. These are smaller suggestions, but please take a look at those as well.

Comment on lines +165 to +169
rank = models.IntegerField(
default=0,
help_text="The relative position of this description in the source",
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Django's order_with_respect_to would create an _order field that is configured exactly like this. We are ordering episodes within each source, so order_with_respect_to = 'source' would make sense.

Since order_with_respect_to is the canonical way of handling this situation and this implementation is essentially replicating it, I would prefer that we take that route.

Comment on lines +8 to +19
class EpisodeOrderInput(InputObjectType):
id = ID(required=True)
rank = Int(required=True)


class UpdateEpisodeOrderMutation(Mutation):
ok = Boolean(required=True)
errors = List(NonNull(LettercraftErrorType), required=True)

class Arguments:
episode_order_data = List(NonNull(EpisodeOrderInput), required=True)

Copy link
Contributor

Choose a reason for hiding this comment

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

Providing the rank in the input isn't really necessary, you can also just use an ordered list of IDs as input.

This will probably make more sense if you use order_with_respect_to, since you could then set the order with something like:

source = Source.objects.get(episodes__in=episode_ids) # should be unique
source.set_episode_order(episode_ids)

This isn't a huge deal though, the current argument structure is superfluous but clear.

Comment on lines +21 to +25
def mutate(cls, root: None, info: ResolveInfo, episode_order_data: list[EpisodeOrderInput]):
# Map Episode IDs to new rank values
id_to_rank: dict[int, int] = {
int(episode.id): episode.rank for episode in episode_order_data
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these type declarations need to be List and Dict instead of list and dict, right?

@XanderVertegaal
Copy link
Contributor Author

I will reopen another PR with order_with_respect_to and with a new interface.

@XanderVertegaal XanderVertegaal deleted the feature/episode-order branch October 30, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Sort episodes by book/chapter/page
2 participants