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

new arg to filter by subclass ref #2105

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tintinthong
Copy link
Contributor

@tintinthong tintinthong commented Jan 30, 2025

Purpose of this PR is to enable spec to choose linkedExamples that link by type

Screenshot 2025-02-05 at 16 36 17 Screenshot 2025-02-05 at 16 41 07

API change

  • <@fields @subclassType={{ref}} />. This means users can control the linksTo and linksToMany editor via this arg. It will only work if you pass in a ref which is indeed a subclass of the type. Otherwise, it defaults to original behaviour
  • I named it subclassType as the arg @lukemelia different from here https://discord.com/channels/584043165066199050/900021750367129641/1334556182001483826 bcos, I think its wrong to overwrite the field entirely. Now the api works st, you only pass the ref that way @fields.author is still true to the type of author but you can for example, <@fields.author @subclassType=bigAuthorRef>. The issue with adding another filter to the every = [ ] in the card chooser is that the index never supports 2 types so we always have to commit to one

@tintinthong tintinthong marked this pull request as draft January 30, 2025 08:50
Copy link

github-actions bot commented Jan 30, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   23m 37s ⏱️ -1s
757 tests +3  755 ✔️ +3  2 💤 ±0  0 ±0 
762 runs  +3  760 ✔️ +3  2 💤 ±0  0 ±0 

Results for commit 9a66cd6. ± Comparison against base commit 28a5621.

♻️ This comment has been updated with latest results.

@tintinthong tintinthong changed the base branch from refactor-links-to-many-component to main February 3, 2025 02:37
@tintinthong tintinthong force-pushed the new-arg-to-filter-catalog-entry-type branch from ab3f885 to c8b1f13 Compare February 3, 2025 02:55
@tintinthong tintinthong force-pushed the new-arg-to-filter-catalog-entry-type branch from c8b1f13 to 9f0fe1c Compare February 5, 2025 04:30
@tintinthong tintinthong marked this pull request as ready for review February 5, 2025 08:37
@tintinthong tintinthong requested review from a team and removed request for a team February 5, 2025 08:47
@tintinthong tintinthong changed the title new arg to filter spec type new arg to filter by subclass ref Feb 6, 2025
Copy link
Contributor

@burieberry burieberry left a comment

Choose a reason for hiding this comment

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

This is all good, but I think it would be fine to make an exception and not check isInsideAncestorChain when the type we're comparing to is the base carddef. isInsideAncestorChain will end up loading all the cards in the ancestor chain when we already know that all card subtypes descend from card def. In that case, you can just confirm via isCardDef for the subtype if we're not sure if the type is a card.

@@ -143,6 +147,7 @@ export class LinksToEditor extends GlimmerComponent<Signature> {

private chooseCard = restartableTask(async () => {
let type = identifyCard(this.args.field.card) ?? baseCardRef;
type = await getNarrowestType(this.args.subclassType, type, myLoader());
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can only call this function if a subclassType arg was provided

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually do you even need to check if the type is the base CardDef? We know that all cards descend from it, so maybe in that case you can skip loading the whole chain and just use the provided subtype.

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