Skip to content

Get file extension and realm info#656

Merged
tintinthong merged 33 commits into
mainfrom
add-extension-and-realm-info
Sep 26, 2023
Merged

Get file extension and realm info#656
tintinthong merged 33 commits into
mainfrom
add-extension-and-realm-info

Conversation

@tintinthong

@tintinthong tintinthong commented Sep 19, 2023

Copy link
Copy Markdown
Contributor

This PR handles getting of file extension AND getting of realm url to display at an arbitrary depth of the file types

Screen.Recording.2023-09-19.at.7.48.51.PM.mov

Now u can see that drafts and base realm exist side by side because its loaded from the type

I actually wanted to move stuff into the adoption chain RESOURCE. But there is a reactivity thing that when I call .type it becomes an infinite loop.

@tintinthong tintinthong changed the title get file extension and realm info Get file extension and realm info Sep 19, 2023
@github-actions

github-actions Bot commented Sep 19, 2023

Copy link
Copy Markdown
Contributor

Test Results

390 tests  ±0   383 ✔️ ±0   5m 7s ⏱️ - 1m 45s
    1 suites ±0       7 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit ffb2ace. ± Comparison against base commit 183c8d5.

♻️ This comment has been updated with latest results.

@tintinthong

tintinthong commented Sep 20, 2023

Copy link
Copy Markdown
Contributor Author

After pairing yesterday, I was thinking deeper about this adoption chain resource and I have a few thoughts.

  • Name Changes. adoptionChainResource-> adoptionChainManager. I am open to naming suggestion.
  • Why I am keeping it. I have decided to keep it because we still need to coordinate the selection of cards within a file. This resource should: 1. handle the selection of the resource card type. schema editor shud just look to it and ask for the selected card type. 2. handle the loading of the card type. That hide the complexity in this resource and cleanup accordingly. This also goes with my understanding of design.
  • Pairing notes. There was a suggestion to treat this more simply to remove the adoption resource and move any selection logic into code mode. Which was good for simplifying the thought process but I think there is purpose for this resource -- particularly for storing selection state and cleaning it up. Also, with the pattern of 2 resources interacting with each other. I think this adoptionChainManager has no business in looking into a resource it is forwarding; I think this was a mistake in even thinking that. It will never look into cardType. The component decides if it wants to render the card type it receives.
  • What the Schema editor should do. The schema editor should just consume a get method -- right now its selectedCardType -- the first module export. In loadInheritanceChain the schema editor should have the returned resource at hand BUT it is still desired that it handles any loading state of the card-type by itself. This will be beneficial so handling any undefined state from the module resource will be abstracted away
  • Future work. The elephant in the room is what about variables and non-cards or fields. This would be a great place to store that coordination logic. I don't think it would be in the same cardType variable but it would in this resource. I think we need to make a good example that exemplifies the entire adoption chain

@tintinthong

tintinthong commented Sep 20, 2023

Copy link
Copy Markdown
Contributor Author
Screen.Recording.2023-09-20.at.3.15.08.PM.mov

Here is an updated screeen recording. Its a mock out of the new design. It makes a lot of better sense now. The resource I created is motivated by this design.

As you can see there is a selection highlight for the export that is selected.

I will incorporate a fuller design in another pr

@tintinthong

Copy link
Copy Markdown
Contributor Author

Ok suggested new names

  • card-inheritance-panel -> module-inspector
  • adoption-chain-resource -> module-inspector-resource

Comment thread packages/host/app/components/operator-mode/definition-container/base.gts Outdated
Comment thread packages/host/app/resources/adoption-chain-manager.ts Outdated
Comment thread packages/host/app/resources/adoption-chain-manager.ts Outdated
Comment thread packages/host/app/resources/adoption-chain-manager.ts Outdated
@tintinthong

Copy link
Copy Markdown
Contributor Author

belurgh fixing test

{{else}}
{{! Module case when visting, eg author.gts }}
<h3>In This File</h3>
{{#each this.elementsInFile as |el|}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the difference between: elements, exports, definitions?

* register a redirect handler that returns redirected response

* revert original change

* fix

* fix

* fix

* fix lint

* fix

* reposition

* clean up & organise useage

* fix file response.url not returning

* fix url parts using pathname

* arg to pass in realm

* make realm url explicit to guard from the wrong use of handle
get adoptionChainTypes() {
return this.args.adoptionChain?.types;
get isCardInstance() {
return this.args.readyFile.url.endsWith('.json');

@tintinthong tintinthong Sep 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@FadhlanR FadhlanR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good for me

Comment thread packages/host/app/resources/adoption-chain-manager.ts Outdated
Comment thread packages/host/app/resources/adoption-chain-manager.ts Outdated

@jurgenwerk jurgenwerk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall it looks good to me, comments for minor data passing simplifications.

<Label class='realm-name' data-test-definition-realm-name>in
{{this.realmName}}</Label>
</div>
{{#if @realmInfo}}

@jurgenwerk jurgenwerk Sep 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider using the RealmInfoProvider component (it was merged recently) so you don't have to pass the realm info into this component and you can get rid of the realmIcon getter

return this.load.isRunning;
}

get selectedElement() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be great to paste this comment into a code comment here so it's clear what elements are https://github.com/cardstack/boxel/pull/656/files#r1332754268

@tintinthong

Copy link
Copy Markdown
Contributor Author

This has a big renaming of Detail Panel . Will resolve it once its merged

}

// forwards the card type resource (doesn't care about the card type resource)
export class AdoptionChainManager extends Resource<AdoptionChainManagerArgs> {

@habdelra habdelra Sep 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm just trying to understand what this resource is actually doing--Manager is such a overloaded term. it's doesn't really help me to know what this thing does. Reading thru this i think what this resource is doing is trying to load all the cards specifically from a module? Maybe CardsFromModuleResource would be a better name for this? the isSelected is super confusing--that feels like it should be state of a component, not this resource. It's not clear how isSelected contributes to this resource other then adding unrelated state

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm i will say that this resource is in charge of managing the selection of elements (elements being exported cards, unexported cards, exported function, exported class). What I intended for this resource is to be consumed by both the detailPanel & the schemeEditor. Perhaps, at one point it will be consumed by monaco to get the location of the selection.

Would it be better to kill the resource and put things in code mode? Or Rename it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that's super confusing to me--why do you need a resource to manage selection? that seems to be something that fits more naturally in a component not a resource. The component should use the resource to understand what all cards exist, and the selection state should live in the component since that is the class that contains the template that consumes the action for selection

@habdelra habdelra Sep 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the fact that you have an action on your resource is a tell that something is not right. the only way to change the state from the outside of the resource should be to pass it in as a thunk that triggers the resource to recompute when state in the thunk changes--not as an action

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually the existence of cards wud be handled by the module resource. And i think it will continue that way. Ok in that case, i will remove this resource and put all logic in the root code mode component

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ive moved the select logic in code. And I also moved the array of cardTypes into code mode but still in a resource pattern. I think this is what we want tho. If it wasn't a resource, we would be recreating cardType everytime. So like you said we still need to load the cards. But I do agree that having it as adoption-chain-manager was confusing because it was not clear what it was doing

@service declare cardService: CardService;
declare loader: Loader;
typeCache: Map<string, Type> = new Map();
moduleMetaCache: Map<string, { extension: string; realmInfo: RealmInfo }> =

@habdelra habdelra Sep 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

perhaps the cache would actually do more if it lived in module scope--then all resources could share it. I would think this kind of information would change very rarely if ever. so probably a cache that could be shared by all resource instances would be more beneficial

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tintinthong
tintinthong merged commit e6318bc into main Sep 26, 2023
@delete-merged-branch
delete-merged-branch Bot deleted the add-extension-and-realm-info branch September 26, 2023 08:56
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.

4 participants