acl-services / paprika

🌢 A robust + accessible UI component library for React applications by Galvanize.
MIT License
54 stars 9 forks source link

Uxd 1757 popover performance - WIP #1152

Closed tristanjasper closed 2 years ago

tristanjasper commented 2 years ago

Purpose πŸš€

Refactor Popover to only render content (& Tip subcomponent) when popover is open and visible. This is to improve overall rendering performance on a page when

Reducing dom nodes that are not visible to the user will improve overall performance for the browser. This change also makes testing easier as previously multiple nodes in popover content elements would be found.

In discussion It may be that this change is the default way that popover now renders it's content. However it is up for discussion as to whether we still support the previous approach where content is still present in the dom (but hidden). We could introduce a new prop to support this, and potentially avoid having to make a major bump to changed components

Still todo Fix 2 failing tests in listbox component which doesn't find active element as previously

Looking into approach using TransitionGroup and keeping content mounted state in Content subcomponent

Notes ✏️

details of code change / secondary purposes of this PR

Updates πŸ“¦

If you have changed a component's source code (not stories, specs, or docs), before merging your branch run yarn changeset. This will prompt you to:

Storybook πŸ“•

http://storybooks.highbond-s3.com/paprika/your-branch-name

Screenshots πŸ“Έ

optional but highly recommended

References πŸ”—

relevant Jira ticket / GitHub issues

changeset-bot[bot] commented 2 years ago

πŸ¦‹ Changeset detected

Latest commit: 00b5f2788c6a177b12b4e612c662c1492d579efd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 21 packages | Name | Type | | --------------------------- | ----- | | @paprika/inline-editors | Major | | @paprika/list-box | Major | | @paprika/list-box-with-tags | Major | | @paprika/popover | Major | | @paprika/search | Major | | @paprika/status-tracker | Major | | @paprika/filter | Patch | | @paprika/list-box-browser | Patch | | @paprika/action-bar | Patch | | @paprika/breadcrumbs | Patch | | @paprika/confirmation | Patch | | @paprika/copy-input | Patch | | @paprika/data-field | Patch | | @paprika/data-header | Patch | | @paprika/date-picker | Patch | | @paprika/date-range-picker | Patch | | @paprika/form-element | Patch | | @paprika/overflow-menu | Patch | | @paprika/side-navigation | Patch | | @paprika/time-picker | Patch | | @paprika/uploader | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tristanjasper commented 2 years ago

Closing to investigate a different approach to not use componentDidUpdate in such a way