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(files): sort not working after changing views #50161

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Jan 13, 2025

  • Resolves: #

Summary

Sorting the tabular view in files breaks if the view is changed twice.

Not a beautiful solution but it works ...

How to reproduce?

  1. Open the files app and sort by size.
  2. Switch to another view, e.g. the "Personal files" view.
  3. Sort again by size -> still works
  4. Switch back to the main files view.
  5. Sort again by size -> sort order doesn't change

Problem: The global $navigation object is not reactive enough and misses some changes.

Before

2025-01-14.13-49-24.mp4

After

2025-01-14.13-51-11.mp4

TODO

  • Record screencasts (before vs. after)
  • Pray that cypress tests still pass
  • Add a cypress test to prevent regressions?

Checklist

@st3iny st3iny self-assigned this Jan 13, 2025
@st3iny
Copy link
Member Author

st3iny commented Jan 13, 2025

/backport to stable30

@st3iny
Copy link
Member Author

st3iny commented Jan 13, 2025

/backport to stable29

@st3iny st3iny force-pushed the fix/files/sort-after-view-change branch from 446823a to 1c3ec2a Compare January 14, 2025 13:16
@st3iny st3iny marked this pull request as ready for review January 14, 2025 13:16
@st3iny st3iny requested a review from skjnldsv as a code owner January 14, 2025 13:16
@st3iny st3iny requested review from susnux and ShGKme January 14, 2025 13:16
Comment on lines +53 to +60
const actualNavigation = getNavigation()
actualNavigation.addEventListener('update', () => {
Vue.set(Navigation, 'views', actualNavigation.views)
Vue.set(Navigation, 'active', actualNavigation.active)
})
actualNavigation.addEventListener('updateActive', () => {
Vue.set(Navigation, 'active', actualNavigation.active)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This should already be handled by the useNavigation composable.
Seems like mixin/filesSorting.ts is the last place that uses the $navigation property directly.

Maybe just replace the usage of this.$navigation then this whole object can be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

There already is a composable for this? I feel stupid now ...

I'll refactor the code then and get rid of the global prop.

@st3iny st3iny added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 14, 2025
@st3iny st3iny marked this pull request as draft January 14, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

2 participants