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

Catalog entry create saving should be consistent #1076

Merged
merged 29 commits into from
Mar 15, 2024

Conversation

tintinthong
Copy link
Contributor

@tintinthong tintinthong commented Mar 7, 2024

This PR saves the card immediately (the card has an id) upon choosing a new catalog entry from cards-grid AND upon choosing a new catalog entry from linking a new card. It doesn't trigger auto-save rather it saves the card purely from the create card action

Previous behaviour
Upon choosing catalog entry (the modal opens), the card is not saved and it doesn't have an id. It is not until you edit a field that the card will be created via auto-save.

Screen.Recording.2024-03-13.at.20.14.51.mov

@tintinthong tintinthong changed the title Cs 6308 catalog entry create should be consistent Catalog entry create saving should be consistent Mar 7, 2024
Copy link

github-actions bot commented Mar 7, 2024

Test Results

530 tests  +3   526 ✔️ +3   7m 19s ⏱️ -3s
    1 suites ±0       4 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 9027398. ± Comparison against base commit 53a5cb2.

♻️ This comment has been updated with latest results.

@tintinthong tintinthong marked this pull request as draft March 7, 2024 12:29
@tintinthong tintinthong force-pushed the cs-6308-catalog-entry-create-should-be-consistent branch from 5e5e539 to a513096 Compare March 11, 2024 02:13
@tintinthong tintinthong force-pushed the cs-6308-catalog-entry-create-should-be-consistent branch from 75b7c8f to 25c3048 Compare March 12, 2024 02:11
Comment on lines -300 to -322
// if this is a newly created card from auto-save then we
// need to replace the stack item to account for the new card's ID
if (!item.card.id && updatedCard.id) {
await item.setCardURL(new URL(updatedCard.id));
}
return;
}

if (item.isLinkedCard) {
this.operatorModeStateService.trimItemsFromStack(item); // closes the 'create new card' editor for linked card fields
} else {
if (!item.card.id && updatedCard.id) {
this.operatorModeStateService.trimItemsFromStack(item);
} else {
this.operatorModeStateService.replaceItemInStack(
item,
item.clone({
request,
format: 'isolated',
}),
);
}
}
Copy link
Contributor Author

@tintinthong tintinthong Mar 13, 2024

Choose a reason for hiding this comment

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

Made a change of fix have "Finish Editing" act consistently

Previous behavior

When you clicked 'Finish Editing' on a newly created linksTo card, the edit modal of the new card will close and the parent card will be displayed with the newly linked card embedded. This seemed unexpected

https://discord.com/channels/584043165066199050/900021750367129641/1216738379425648710

Boxel Meeting Notes
So only after looking at this image I recall that Chris did say that the UI looks wonky for this and he wanted the Previous behavior

Screenshot 2024-03-13 at 20 19 42

But imo, the buttons make more sense with this change. If you want to skip to just seeing the parent card with the newly created card embedded, you can just click "Close"

super(owner, args);
// initializes to false
if (this.args.model === undefined) {
this.args.set(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment here

super(owner, args);
// initializes to 'actual'
if (!this.args.model) {
this.args.set('actual');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was triggering auto-save. Which is why there was an inconsistency in the behaviour of the new cards. They were "saved" sometimes upon being created.

Copy link
Contributor Author

@tintinthong tintinthong Mar 13, 2024

Choose a reason for hiding this comment

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

I renamed this entire file to solve issue with sourcemap and debugging. Basically, I wouldn't be able to trace my debugger properly since there are 2 files operator-mode-test.

Positional: [model: boolean | null, inputType: boolean];
get checkedId() {
return String(this.args.model);
}
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before
Screenshot 2024-03-13 at 20 33 27

After
Screenshot 2024-03-13 at 20 33 08

Comment on lines +40 to +48
static async [deserialize]<T extends BaseDefConstructor>(
this: T,
val: any,
): Promise<BaseInstanceType<T>> {
if (val === undefined || val === null) {
return false as BaseInstanceType<T>;
}
return Boolean(val) as BaseInstanceType<T>;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we may possibly have null in the JSON but may want to display a default value for certain components. We stub a default value via the deserialize hook. This doesn't trigger editing of the card in the constructor.

I think there is compelling use-case for this. For example, like when displaying calendar date

@@ -204,58 +216,32 @@ class ImageSizeField extends FieldDef {
static edit = class Edit extends Component<typeof this> {
<template>
<div class='radio-group' data-test-radio-group={{@fieldName}}>
<label for='{{this.radioGroup}}_actual'>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before
Screenshot 2024-03-13 at 20 41 42

After

Screenshot 2024-03-13 at 20 38 55

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, I shud work on fixing the component to be vertical vs horizontal given the spacing of the parent.

@tintinthong tintinthong marked this pull request as ready for review March 13, 2024 12:44
@tintinthong tintinthong requested a review from ef4 March 13, 2024 12:44
@@ -167,6 +167,7 @@ export default class InteractSubmode extends Component<Signature> {
doc,
relativeTo,
);
await here.cardService.saveModel(here, newCard);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I do sus if this is the correct owner?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be ok, it bascially means that the live updates for the newCard will continue to be received while the interact-submode-component is in scope.

@tintinthong tintinthong merged commit e26d5b8 into main Mar 15, 2024
18 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cs-6308-catalog-entry-create-should-be-consistent branch March 15, 2024 07:33
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