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.

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

refactor!: Drop clickOutsideOptions mixin and refactor using components #6429

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open

refactor!: Drop clickOutsideOptions mixin and refactor using components #6429

wants to merge 2 commits into from
+256 −370

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 23, 2025

☑️ Resolves

  • Drop clickOutsideOptions mixin and refactor using components.
    • Refactored to use <script setup> with proper Typescript types and slot definitions.
    • Following components which used the mixin were refactored to not need it anymore:
      • NcHeaderMenu
      • NcAppNavigationSettings

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux
refactor!: Drop clickOutsideOptions mixin and refactor using compon…
e6f67d5 
…ents

Following components which used the mixin were refactored to not need it
anymore. Also refactored to use `<script setup>` with proper Typescript
types and slot definitions.

- `NcHeaderMenu`
- `NcAppNavigationSettings`

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux added 3. to review Waiting for reviews 💥 breaking PR that requires a new major version vue 3 Related to the vue 3 migration refactor ♻️ Pull request that is neither a fix nor a feature labels Jan 23, 2025
@susnux susnux added this to the 9.0.0-alpha.7 milestone Jan 23, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Jan 23, 2025

  • Refactored to use <script setup> with proper Typescript types and slot definitions.

Is it required to remove the mixin? If not, could we separate PR with mixing removing and component rewriting?

@susnux
fixup!
6655a57 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
Copy link
Contributor Author

susnux commented Jan 23, 2025

Is it required to remove the mixin? If not, could we separate PR with mixing removing and component rewriting?

No but the refactor is needed before removal. So after that it is not needed anymore, so we can do it directly.

@ShGKme ShGKme mentioned this pull request Jan 26, 2025
Open
st3iny
st3iny approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@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.

Looks good

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

@st3iny st3iny st3iny approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

@ShGKme ShGKme Awaiting requested review from ShGKme

@Antreesy Antreesy Awaiting requested review from Antreesy

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
3. to review Waiting for reviews 💥 breaking PR that requires a new major version refactor ♻️ Pull request that is neither a fix nor a feature vue 3 Related to the vue 3 migration
Projects
None yet
Milestone
9.0.0-alpha.7
Development

Successfully merging this pull request may close these issues.
3 participants
@susnux @ShGKme @st3iny