Skip to content

fix(files): highlight previous folder on history up #53285

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.

Sign up for GitHub

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

Jump to bottom
Merged
skjnldsv merged 2 commits into from
Jun 3, 2025
Merged

fix(files): highlight previous folder on history up #53285

skjnldsv merged 2 commits into from
Jun 3, 2025

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 3, 2025

Fix #53031

@skjnldsv skjnldsv added this to the Nextcloud 32 milestone Jun 3, 2025
@skjnldsv skjnldsv self-assigned this Jun 3, 2025
@skjnldsv skjnldsv requested a review from a team as a code owner June 3, 2025 11:33
@skjnldsv skjnldsv requested review from susnux and removed request for a team June 3, 2025 11:33
@skjnldsv skjnldsv added the 3. to review Waiting for reviews label Jun 3, 2025
@skjnldsv skjnldsv requested review from artonge and sorbaugh June 3, 2025 11:33
@skjnldsv skjnldsv added feature: files papercut Annoying recurring UX issue with possibly simple fix. labels Jun 3, 2025
@github-project-automation github-project-automation bot moved this to 🏗️ In progress in 📁 Files team Jun 3, 2025
@skjnldsv skjnldsv force-pushed the fix/files-position-navigation branch from a3873e7 to bcfbd42 Compare June 3, 2025 11:33
@skjnldsv skjnldsv changed the title fix(files): highlight previosu folder on history up fix(files): highlight previous folder on history up Jun 3, 2025
susnux
susnux reviewed Jun 3, 2025
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

How does this belong to the hot key service? Seems to be generic for all navigation?

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 3, 2025

How does this belong to the hot key service? Seems to be generic for all navigation?

Right, it's kind of hijacking the same shortcut as alt+up.
Pressing the back button is also kind of a hot key (I have dedicated mouse and keyboard buttons for it) 🤷

@susnux
Copy link
Contributor

susnux commented Jun 3, 2025

Pressing the back button is also kind of a hot key (I have dedicated mouse and keyboard buttons for it) 🤷

Yes but this is basic routing / history handling, no? Would this not better suit the router directly?
I would not expect it in the hot key service to be honest.

@susnux
Copy link
Contributor

susnux commented Jun 3, 2025

(approved as the code itself is good, just the location 🤷 )

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 3, 2025

Pressing the back button is also kind of a hot key (I have dedicated mouse and keyboard buttons for it) 🤷

Yes but this is basic routing / history handling, no? Would this not better suit the router directly? I would not expect it in the hot key service to be honest.

Fair to me!
let me move it :)

@skjnldsv skjnldsv force-pushed the fix/files-position-navigation branch 2 times, most recently from a382619 to 5b5d5db Compare June 3, 2025 15:08
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 3, 2025

And with cypress tests

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 3, 2025

/compile rebase

@nextcloud-command nextcloud-command force-pushed the fix/files-position-navigation branch from 5b5d5db to 9a30a8f Compare June 3, 2025 15:40
@nextcloud-command nextcloud-command requested a review from a team as a code owner June 3, 2025 15:40
@skjnldsv skjnldsv enabled auto-merge June 3, 2025 15:53
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 3, 2025

/backport 5974649 to stable31

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 3, 2025

/backport 5974649 to stable30

susnux
susnux approved these changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Nice 🚀

@skjnldsv skjnldsv merged commit 97a9a68 into master Jun 3, 2025
124 checks passed
@skjnldsv skjnldsv deleted the fix/files-position-navigation branch June 3, 2025 16:36
This was referenced Jun 3, 2025
Closed
Merged
jancborchardt
jancborchardt reviewed Jun 4, 2025
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@skjnldsv thanks a bunch! :) Just to clarify, the issue #53031 was mostly about "Scroll position within folders is lost on navigation", is that also fixed with this pull request?

@skjnldsv skjnldsv moved this from 🏗️ In progress to ☑️ Done in 📁 Files team Jun 4, 2025
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 4, 2025

@skjnldsv thanks a bunch! :) Just to clarify, the issue #53031 was mostly about "Scroll position within folders is lost on navigation", is that also fixed with this pull request?

Yes, should be :)

@jancborchardt
Copy link
Member

jancborchardt commented Jul 30, 2025 •
edited
Loading

@skjnldsv is this deployed on daily yet? I just tried with both:

  • Going back via browser controls
  • Going up to parent folder via breadcrumbs

And both still end up at the top of the parent folder. Let me know if it’s not on daily yet. :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Jul 30, 2025

No clue what version daily is.
But it should be deployed on c.nc.com already (Nextcloud 31.0.6)

@susnux
Copy link
Contributor

susnux commented Jul 30, 2025

On c.nc.com both work (and also on current master branch)

@jancborchardt
Copy link
Member

jancborchardt commented Jul 31, 2025

Just tested again on c.nc, and while the previous folder is highlighted, the scroll position still ends up on the very top instead of keeping the position → as asked in the original issue, ref #53285 (review)

I also cleared cache and everything, not sure what issue it could be?

@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jancborchardt jancborchardt jancborchardt left review comments

@susnux susnux susnux approved these changes

@miaulalala miaulalala miaulalala approved these changes

@artonge artonge Awaiting requested review from artonge artonge is a code owner automatically assigned from nextcloud/server-frontend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-frontend

Assignees

@skjnldsv skjnldsv

Labels

3. to review Waiting for reviews enhancement feature: files papercut Annoying recurring UX issue with possibly simple fix.

Projects

📁 Files team
Status: ☑️ Done

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

Scroll position within folders is lost on navigation

6 participants

@skjnldsv @susnux @jancborchardt @miaulalala @nextcloud-command