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

Frontend bugfixes: linked users table on or-asset-viewer #1284

Merged
merged 3 commits into from
May 22, 2024

Conversation

MartinaeyNL
Copy link
Contributor

@MartinaeyNL MartinaeyNL commented Apr 5, 2024

Fix for #1263 where only 10 items were present in the table due to a pagination issue.
Also fixed the roles being incorrect in the linked users table. (#1211)

Full changelog;

  • Fix for or-mwc-table where a pagination filter was applied without pagination being enabled.
  • Fix for or-mwc-table where pagination controls were shown without pagination being enabled.
  • Issue where the linked users table in or-asset-viewer showed all roles instead of the assigned roles.

…pagination issue. Also fixed the roles being incorrect in the linked users table.
@MartinaeyNL MartinaeyNL linked an issue Apr 5, 2024 that may be closed by this pull request
@MartinaeyNL MartinaeyNL changed the title Frontend Bugfixes: linked users table on or-assert-viewer Frontend bugfixes: linked users table on or-asset-viewer Apr 5, 2024
@richturner
Copy link
Member

Just paginating on in memory data is a bit limiting, the caller should probably deal with pagination (data and/or visual); a better pagination interface (arrow function) should be provided and pagination enabled can be deduced by whether this field/property is set or not.

@MartinaeyNL
Copy link
Contributor Author

MartinaeyNL commented May 14, 2024

Sorry for this late response on the PR;

I agree that an exposed paginationFilter property, where the consumer / parent component decides what rows to show,
would be a nice addition. It would enhance flexibility and control. However, I would keep the additional enable property,
as there are more configuration options, such as setting the amount of rows per page.

Instead of implementing this functionality now, I suggest to create a new issue for this.
I'd rather have this simple fix merged first, and work on that later 😉

@richturner @DonWillems

@MartinaeyNL MartinaeyNL merged commit e09c676 into master May 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants