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

[LPDC-1277] Add modal popup to see existing instances before creating new one #19

Open
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

wolfderechter
Copy link
Contributor

ID

LPDC-1277

Description

Add modal popup to warn the user when they are about to create an instance that already exists, show the existing instances so the user can consolidate them.

How to test

  1. Go to frontend
  2. Click the Product of dienst toevoegen button
  3. Click on different concepts:
    • Those that already have an existing instance should show the popup informing the user
    • Those without an existing instance should just create the instance

@wolfderechter wolfderechter added the enhancement New feature or request label Feb 27, 2025
@@ -152,14 +152,12 @@
{{else if this.hasResults}}
<Content.body as |concept|>
<td>
<AuLink
@route="public-services.concept-details"
Copy link
Contributor

@Windvis Windvis Mar 3, 2025

Choose a reason for hiding this comment

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

It seems it's now no longer possible to preview a concept before creating an instance. Was that an intentional change?

if we want to skip the preview, I think that only makes sense for concepts that are already instantiated? So we could conditionally show the preview link if there are no instances yet, or the button that triggers the modal if there are already some instances.

I think it make more sense to simply redirect to the preview page from the modal though, but maybe double check with the design team. Or move the modal to the preview page instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was indeed an oversight, I wasn't sure what it did. I've added the preview back in

Copy link
Contributor

Choose a reason for hiding this comment

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

I would do it conditionally here, instead of in the modal. That way the API call in the modal doesn't need to happen when it isn't needed and links are generally better because users can open those in new tabs if they want.

{{#if concept.displayConfiguration.isInstantiated}}
  {{! modal button }}
 <AuButton ...
{{else}}
  {{! the original AuLink component}}
  <AuLink ...
{{/if}}

But I'm still not convinced we should display the modal here (or a modal at all). I think it might be better to update the details preview page instead, but again, ideally this would get looked at by the design team..

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'm not sure why but the isInstantiated doesn't seem to be accurate, some of the instances are displayed correctly but some are not..

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's a bigger problem then. I can't remember how that part works exactly, or where it's updated. But that would also mean that users would see the modal even if there is no "toegevoegd" label next to the concept. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, I have not really used the app before so I'm not sure if I'm missing something (maybe it needs some time to update?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cc: @mirdono

},
});
} else {
this.createPublicService.perform(conceptId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to do this without showing the user the preview first? Also, if we check the concept.displayConfiguration.isInstantiated flag in the template before displaying the modal button, this else block should never be reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean with this, but I've added the preview queryParam

Comment on lines 11 to 20
<span class="au-u-h4 au-u-medium">Bestaande instanties</span>
{{#each @data.existingInstances as |instance|}}
<p>
<AuLink class="au-u-margin-right" @route="public-services.details" @model={{instance.id}} @query={{hash
preview=true}} {{on "click" this.close}}>
{{instance.nameNl}}
</AuLink>
gewijzigd op ({{moment-format instance.dateModified "DD-MM-YYYY - HH:mm"}})
</p>
{{/each}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +21 to +25
<AuButton class="au-u-margin-top au-u-padding-none" @skin="link" @disabled={{this.createNewInstance.isRunning}}
@loading={{this.createNewInstance.isRunning}} @loadingMessage="Nieuwe instantie aan het aanmaken" {{on "click"
(perform this.createNewInstance)}}>
Nieuwe instantie maken
</AuButton>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was discussed in the Jira issue, but I would double check with the UX team. It feels like an anti-pattern to make things "less obvious" on purpose to discourage the use of it.

It just looks weird to me at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants