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(dashboard): performance and refactoring #41065

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 23, 2023

Summary

⚠️ Chained on #41063

Polish a bit dashboard app:

  • Decrease duplication of dashboard v1/v2
  • Polish styles to replace classes like panel-activity--header--icon--description and very nested selectors with selectors by tags
  • Improve dragging performance:
    • Dragging dashboard was very laggy
    • Hidden icon description with .hidden-visually has transforms that makes it terrible in performance with a combination of Draggable
    • This description is not needed, icon is hidden for a11y
    • Remove the hidden description
Before After
dashboard-laggy dashboard-after

Checklist

@skjnldsv
Copy link
Member

@ShGKme ShGKme (Grigorii K. Shartsev) requested review from st3iny, susnux, JuliaKirschenheuter, skjnldsv, Pytal and szaimen 16 hours ago

Not ready for review?
image

@st3iny
Copy link
Member

st3iny commented Oct 24, 2023

@ShGKme You might have accidentally committed a dev build :D

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. I couldn't spot regressions.

Nice work!

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
- aria-labelledby is not neede here, it is a hidden icon
- visually-hidden has transformations that has huge performance impact
  in combination with other transformations, for example, on draggable

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 24, 2023

Not ready for review?

@ShGKme You might have accidentally committed a dev build :D

It was in draft waiting for the base branch #41063 to be merged.

Rebased now and ready for review.

@ShGKme ShGKme force-pushed the fix/dashboard--performance-and-refactoring branch from 86f8b4f to da88d22 Compare October 24, 2023 08:54
@ShGKme ShGKme marked this pull request as ready for review October 24, 2023 08:54
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 24, 2023
Comment on lines +17 to 28
<div v-for="panelId in layout" :key="panels[panelId].id" class="panel">
<h2 class="panel__header">
<span aria-hidden="true" class="panel__header-icon" :class="getWidgetIconClass(panels[panelId])" />
{{ getWidgetTitle(panels[panelId]) }}
</h2>
<div class="panel__content" :class="{ loading: !isApiWidgetV2(panels[panelId].id) && !panels[panelId].mounted }">
<ApiDashboardWidget v-if="isApiWidgetV2(panels[panelId].id)"
:widget="apiWidgets[panels[panelId].id]"
:data="apiWidgetItems[panels[panelId].id]"
:loading="loadingItems" />
<div v-else :ref="panels[panelId].id" :data-id="panels[panelId].id" />
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Seeing the amount of scoping done here as well as methods used that could be computed, we would benefit from this being a chidren Vue Component directly :)

<ApiDashboardPanel v-for="panelId in layout"
	:key="panels[panelId].id"
	:panel="panels[panelId]" />

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

super nice!

@ShGKme ShGKme added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 26, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants