Skip to content

fix: Use resize observer to calculate menubar icon limit - #4637

Merged
juliusknorr merged 2 commits into
mainfrom
bugfix/noid/resize-menubar
Aug 7, 2023
Merged

fix: Use resize observer to calculate menubar icon limit#4637
juliusknorr merged 2 commits into
mainfrom
bugfix/noid/resize-menubar

Conversation

@juliusknorr

@juliusknorrjuliusknorr commented Aug 5, 2023

Copy link
Copy Markdown
Member

Signed-off-by: Julius Härtl jus@bitgrid.net

📝 Summary

Make the icon limit recalculation more reliable by using a resize observer on the actual element and makes the code much cleaner

@vueuse/core is already a dependency through @nextcloud/vue

Fixesnextcloud/deck#4970

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress

cypressBot commented Aug 5, 2023

Copy link
Copy Markdown

Passing run #11537 ↗︎

014920Flakiness0

Details:

fix: Use resize observer to calculate menubar icon limit
Project: TextCommit:088ef70a35
Status: PassedDuration: 03:56 💡
Started: Aug 7, 2023 1:38 PMEnded: Aug 7, 2023 1:42 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@max-nextcloudmax-nextcloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code changes look good. So much cleaner indeed.
I did not give it a try. Happy to hear it also fixes things in deck.
I have one minor question in the review.
Also... backports?

Comment threadsrc/components/Menu/MenuBar.vue
@juliusknorr
juliusknorrforce-pushed the bugfix/noid/resize-menubar branch 2 times, most recently from 42fdb59 to 2766bfbCompareAugust 7, 2023 06:18
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

/compile

@mejo-mejo- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for looking into this! We still a listener on resize in getClientWidth() at src/store/plugin.js and in src/mixins/isMobile.js.

Also in src/components/Editor/EditorOutline.vue we implement our own $resizeObserver.

Maybe it would be good to consolidate all of them? 🤔

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Sounds like a good idea. For the isMobile mixin we can actually rely on the one from @nextcloud/vue as it has a shared state and doesn't add another listener per usage.

I'd do that in a follow up to have the bugfix isolated for backports

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Need to double check, cypress failure looks like it could be related not showing the menubar on direct editing.

https://cloud.cypress.io/projects/hx9gqy/runs/11522/test-results/9f7c840a-7788-4409-a6d1-cfd42c58408c

@juliusknorr
juliusknorrforce-pushed the bugfix/noid/resize-menubar branch from 89a51a8 to 6e3ecf0CompareAugust 7, 2023 12:41
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Pushed a fix to set the ready state in any case as for plain text files we don't have a resize observer as the menubar isn't there.

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

/compile amend /

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@mejo-
mejo-force-pushed the bugfix/noid/resize-menubar branch from 6e3ecf0 to 180cc39CompareAugust 7, 2023 13:27
@mejo-

mejo- commented Aug 7, 2023

Copy link
Copy Markdown
Member

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliusknorr
juliusknorr merged commit 44235cb into mainAug 7, 2023
@juliusknorr
juliusknorr deleted the bugfix/noid/resize-menubar branch August 7, 2023 13:42
@mejo-

mejo- commented Aug 7, 2023

Copy link
Copy Markdown
Member

/backport 180cc39 to stable27

@mejo-

mejo- commented Aug 7, 2023

Copy link
Copy Markdown
Member

/backport 180cc39 to stable26

@backportbot-nextcloud

Copy link
Copy Markdown

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27
# Create the new backport branch
git checkout -b fix/foo-stable27
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud

Copy link
Copy Markdown

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin stable26
# Create the new backport branch
git checkout -b fix/foo-stable26
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@mejo-

mejo- commented Aug 7, 2023

Copy link
Copy Markdown
Member

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releasebugSomething isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Not all formatting options show up

4 participants

@juliusknorr@mejo-@max-nextcloud@nextcloud-command