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

[next] chore(NcBreadcrumbs): simplify code #5068

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Jan 14, 2024

☑️ Resolves

This PR simplifies the code of NcBreadcrumbs.

  • 825607a changes the render function to use a single Array.splice instead of slice -> push -> slice -> concat. This should speed up the render function a bit.
  • 1ddda89 changes the breadcrumbsRefs object to be an array, which safes us from running Object.values at two spots.

I didn't send this PR against master to not introduce any issues in the stable branch. Also, merging changes to vue2 render functions into next is always a pain. In case you insist, we could of course also improve it for master.

No visual or functional changes.

Best viewed as https://github.com/nextcloud-libraries/nextcloud-vue/pull/5068/files?diff=unified&w=1

@raimund-schluessler raimund-schluessler changed the title Chore/noid/breadcrumbs cleanup [next] chore(NcBreadcrumbs): simplify code Jan 14, 2024
@raimund-schluessler raimund-schluessler added the feature: breadcrumbs Related to the breadcrumbs components label Jan 14, 2024
@raimund-schluessler raimund-schluessler force-pushed the chore/noid/breadcrumbs-cleanup branch from 0bf0bc0 to 1ddda89 Compare January 14, 2024 13:20
@raimund-schluessler raimund-schluessler added the vue 3 Related to the vue 3 migration label Jan 14, 2024
@raimund-schluessler raimund-schluessler added the 3. to review Waiting for reviews label Jan 14, 2024
@raimund-schluessler raimund-schluessler marked this pull request as ready for review January 14, 2024 14:13
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.

looks good to me

@raimund-schluessler raimund-schluessler force-pushed the chore/noid/breadcrumbs-cleanup branch from 1ddda89 to c9d2242 Compare January 31, 2024 08:00
@raimund-schluessler raimund-schluessler marked this pull request as draft January 31, 2024 08:05
@raimund-schluessler raimund-schluessler force-pushed the chore/noid/breadcrumbs-cleanup branch from c9d2242 to a57a912 Compare January 31, 2024 08:08
@raimund-schluessler raimund-schluessler marked this pull request as ready for review January 31, 2024 08:12
@raimund-schluessler
Copy link
Contributor Author

Resolved conflicts.

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Seems legit, code changes should be equivalent
Haven't work with array refs in Vue 3, but know they should work 🦭

@raimund-schluessler
Copy link
Contributor Author

These random Cypress failures are annoying. Six runs now and always a different test failing.

@raimund-schluessler raimund-schluessler merged commit 9116758 into next Jan 31, 2024
17 checks passed
@raimund-schluessler raimund-schluessler deleted the chore/noid/breadcrumbs-cleanup branch January 31, 2024 11:57
@susnux
Copy link
Contributor

susnux commented Jan 31, 2024

These random Cypress failures are annoying. Six runs now and always a different test failing.

Yes that is super annoying, Cypress knows of that flaky tests for couple of month but is not considered to be something worth to fix.
@ShGKme mentioned PlayWright to me some time ago, since then I only use that for my personal projects, much less annoying 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feature: breadcrumbs Related to the breadcrumbs components vue 3 Related to the vue 3 migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants