Conversation
|
Should we write tests for this? As of now we do not have tests in this part of the plugin. |
tmenzel27
left a comment
tmenzel27
left a comment
There was a problem hiding this comment.
I think we should use the Barrel Pattern also for components.
There was a problem hiding this comment.
Just in general we have now some code inside the UI comonents right but i guess its okay?
There was a problem hiding this comment.
hmm, good question. I feel like its pretty much connected to the ui since its only validating. moving this towards headless feels a bit wrong. Wdyt?
There was a problem hiding this comment.
It depends if it can be used headless like queries it should be moved to headless.
Reset.ts is tough but i guess it could stay there.
I am also not a fan of a lib folder like that isn't that usually what for us headless is?
Are the Types we have here not commonly used?
For the matching i have also moved it to that folder inside headless, but i get why it would be nice to have that close to each other.
There was a problem hiding this comment.
The types are really only scoped for the form, they define the formData.
I moved the query function, since this one really makes sense in the headless part of the plugin.
I believe the other functions are so closely linked to the form and cant really be used by any other parts of the plugin, this is why I would keep them there.
I may rename the lib folder to something more descriptive, less blackboxy.
| <MultiApButton | ||
| {isMultiApMode} | ||
| hasValidInput={isMultiApMode ? hasCurrentAp : hasValidIed} | ||
| onEnterMultiApMode={enterMultiApMode} | ||
| onAddAccessPoint={addAccessPoint} | ||
| /> | ||
|
|
There was a problem hiding this comment.
Wouldn't it be easier if we are always in "multiple AP" mode and depending on how many the user adds, that is how many are created? E.g. if you want just one, then just add one
There was a problem hiding this comment.
the way the ui is designed right now, we do first expect a single ap, only if the user "opts-in" the ui should offer multi ap creation.
| export function validateSubmission({ | ||
| ied, | ||
| accessPoints, | ||
| xmlDocument | ||
| }: ValidateSubmissionParams): string | null { | ||
| const trimmedIedName = ied.name.trim() | ||
|
|
||
| if (ied.isNew) { | ||
| if (!trimmedIedName) { | ||
| return 'IED name is required when creating a new IED' | ||
| } | ||
| if (queryIedExists(xmlDocument, trimmedIedName)) { | ||
| return `IED "${trimmedIedName}" already exists` | ||
| } | ||
| } else { | ||
| if (!trimmedIedName) { | ||
| return 'Please select an existing IED or create a new one' | ||
| } | ||
| } | ||
|
|
||
| if (accessPoints.length === 0) { | ||
| return 'At least one Access Point is required' | ||
| } |
There was a problem hiding this comment.
Writing form validtion can get messy and confusing quickly, so it might be worthwhile to look into using a form library or writing your own.
I haven't used any form library in svelte so I can't make a recommendation, but you could take a look at these https://svelte.dev/packages#forms.
There was a problem hiding this comment.
will do so using tanstack form management in a future pr.
tmenzel27
left a comment
tmenzel27
left a comment
There was a problem hiding this comment.
LGTM. There is nothing that needs to be done before merging this is there?
3 participants
🗒 Description
Expand the logic and UI of the create-ied-and-ap dialog to allow the creation and validation of multi access points
📷 Demo screen recording or Screenshots
📋 Checklist
You may remove tasks that do not make sense in this PR.
🔦 Useful commits
You can add specific commits which are worth taking a look into
❌ Link issue(s) to close - hint
closes #570