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

Fix #1721 #1722

Merged
merged 8 commits into from
Jul 31, 2024
Merged

Fix #1721 #1722

merged 8 commits into from
Jul 31, 2024

Conversation

nsoeth
Copy link

@nsoeth nsoeth commented Dec 7, 2023

Q A
Bug fix? [ ]
New feature? [ ]
New sample? [x]
Related issues? fixes #X, partially #Y, mentioned in #Z

What's in this Pull Request?

This PR will fix #1721

Guidance

  • You can delete this section when you are submitting the pull request.
  • Please update this PR information accordingly. We'll use this as part of our release notes in monthly communications.
  • Please target your PR to dev branch.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for the first contribution to this project.

@michaelmaillot
Copy link
Collaborator

Hi @nsoeth,

Thanks for this PR!

Regarding the addition of $top query operation, I'd suggest to make it available as optional because it depends on use case and maybe it would not fit for everyone to load 5000 items at once.

Since the DynamicForm lookup field relies on ListItemPicker control, and in order to be available as a field override prop for the DynamicForm, here are the different files to update according to me:

Like this, the developer will be free to set the number of items he wants to get 🙂

A suggestion of prop name could be itemsQueryCountLimit or ODataTopCount for example but feel free to suggest!

Let me know if something's not clear or if you need help.

@nsoeth
Copy link
Author

nsoeth commented Feb 27, 2024

Hi @michaelmaillot ,
Thanks for your suggestions, I updated the PR.

Kind regards,
Niels

Copy link
Collaborator

@michaelmaillot michaelmaillot left a comment

Choose a reason for hiding this comment

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

Hi @nsoeth , I've submitted a fews changes to be done, let me know if you want any kind of help 🙂

Edit: as a guidance, I suggest you to run the solution locally: you'll be able to test the DynamicForm through the included ControlsTests webpart. Or you can test it on your consuming solution with npm link command. Feel free to reach out if you need assistance.

@@ -221,7 +221,8 @@ export default class SPService implements ISPService {
filterString?: string,
substringSearch: boolean = false,
orderBy?: string,
cacheInterval: number = 1): Promise<any[]> { // eslint-disable-line @typescript-eslint/no-explicit-any
cacheInterval: number = ICON_GENERIC_16,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this should be:

cacheInterval: number = 1,

@@ -259,7 +260,7 @@ export default class SPService implements ISPService {
return filteredItems;
}

apiUrl = `${webAbsoluteUrl}/_api/web/lists('${listId}')/items?$select=${keyInternalColumnName || 'Id'},${internalColumnName},FieldValuesAsText/${internalColumnName}&$expand=FieldValuesAsText&$orderby=${orderBy}${filterString ? '&$filter=' + filterString : ''}`;
apiUrl = `${webAbsoluteUrl}/_api/web/lists('${listId}')/items?$select=${keyInternalColumnName || 'Id'},${internalColumnName},FieldValuesAsText/${internalColumnName}&$expand=FieldValuesAsText&$orderby=${orderBy}${filterString ? '&$filter=' + filterString : ''}${top ? `&$top=${top}` : ''};
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a missing backtick (`) at the end of this line, which raises errors on this file during the build.

The top prop shouldn't be placed here but here.

return [{ key: result[fieldName].ID, name: result[fieldName][lookupFieldName || 'Title'] }];
let value = result[fieldName][lookupFieldName || 'Title'];
const dateVal = Date.parse(value);
if (dateVal !== NaN) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't have to update this line because the getLookupValue is not used and should be deleted.

src/services/SPService.ts Outdated Show resolved Hide resolved
lookups.push({ key: singleItem.ID, name: singleItem[lookupFieldName || 'Title'] });
let value = singleItem[fieldName][lookupFieldName || 'Title'];
const dateVal = Date.parse(value);
if (dateVal !== NaN) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to compare with the type function:

if (!Number.isNaN(dateVal))

@@ -221,7 +221,8 @@ export default class SPService implements ISPService {
filterString?: string,
substringSearch: boolean = false,
orderBy?: string,
cacheInterval: number = 1): Promise<any[]> { // eslint-disable-line @typescript-eslint/no-explicit-any
cacheInterval: number = ICON_GENERIC_16,
top?: number): Promise<any[]> { // eslint-disable-line @typescript-eslint/no-explicit-any
Copy link
Collaborator

Choose a reason for hiding this comment

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

top property should be placed before cacheInterval as this one has a default value. Otherwise, there will be a confusion if itemitemsQueryCountLimit prop is filled.

});
}
//single select lookups are objects
else {
const singleItem = result[fieldName];
lookups.push({ key: singleItem.ID, name: singleItem[lookupFieldName || 'Title'] });
let value = singleItem[fieldName][lookupFieldName || 'Title'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be singleItem[lookupFieldName || 'Title'] because you've already iterate into the fieldName data.

result[fieldName].forEach(element => {
lookups.push({ key: element.ID, name: element[lookupFieldName || 'Title'] });
result[fieldName].forEach(element => {
let value = element[fieldName][lookupFieldName || 'Title'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be element[lookupFieldName || 'Title'] because you've already iterate into the fieldName data.

@michaelmaillot
Copy link
Collaborator

Hi @nsoeth,

Are you still working on this one? Do you need any kind of help?

@nsoeth
Copy link
Author

nsoeth commented May 29, 2024

Hi @michaelmaillot
Sorry, lot of things to do the last few months but I updated the PR according to your review.

@michaelmaillot michaelmaillot merged commit 783568e into pnp:dev Jul 31, 2024
1 check passed
@michaelmaillot
Copy link
Collaborator

Merged manually, thank you!

@michaelmaillot michaelmaillot added the status:fixed-next-drop Issue will be fixed in upcoming release. label Jul 31, 2024
@michaelmaillot michaelmaillot added this to the 3.19.0 milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:fixed-next-drop Issue will be fixed in upcoming release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants