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: convert useComputedClass to defineClass #725

Merged
merged 45 commits into from
Feb 1, 2024

Conversation

mlmoravek
Copy link
Member

@mlmoravek mlmoravek commented Jan 6, 2024

Fixes #709

Currently, each time a computed is triggered, the useComputedClass composable is triggered for each component class. This causes the getCurrentInstance() method to be called during the computed hook.

I created a new composable called defineClasses which will replace the computed component classes and return a reactive ref value. This way the getCurrentInstance() method got only triggered once synchronous during setup. The class is calculated synchronous during setup too and every time the suffix is changed by a watcher. Another watcher is used to change the apply state of the class if given. However, this doesn't require the class to be recalculated.

This also improves the update time of a computed class, because before, whenever any property or ref value was used to compute the class got updated, the computation for each class calculation used in the computed const got triggered. Now, whenever a property changes, only the relevant class calculation is triggered.

For each component, the "Computed Component Classes" section must be refactored to use the new feature. No component logic or template should be touched.

Proposed Changes

  • refactor computed useComputedClass to defineClasses
  • Update docs to use Vue 3.4 and latest vitepress version
  • Extend basic button docs example

@mlmoravek mlmoravek added bug Something isn't working help wanted Extra attention is needed refactoring This involves some refactoring of existing code. labels Jan 6, 2024
@mlmoravek mlmoravek self-assigned this Jan 6, 2024
Copy link

netlify bot commented Jan 6, 2024

Deploy Preview for oruga-documentation-preview ready!

Name Link
🔨 Latest commit f41794a
🔍 Latest deploy log https://app.netlify.com/sites/oruga-documentation-preview/deploys/65bbe85cbdd71a0008156669
😎 Deploy Preview https://deploy-preview-725--oruga-documentation-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jan 6, 2024

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (68e4dee) 52.12% compared to head (f41794a) 54.62%.

Files Patch % Lines
...ckages/oruga-next/src/composables/defineClasses.ts 77.52% 20 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #725      +/-   ##
===========================================
+ Coverage    52.12%   54.62%   +2.49%     
===========================================
  Files           32       32              
  Lines         1385     1461      +76     
  Branches       512      519       +7     
===========================================
+ Hits           722      798      +76     
  Misses         663      663              
Flag Coverage Δ
oruga-next 54.62% <88.88%> (+2.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mlmoravek mlmoravek removed the help wanted Extra attention is needed label Jan 24, 2024
@mlmoravek mlmoravek changed the title WIP: fix: refactor useComputedClass to defineClass fix: convert useComputedClass to defineClass Jan 30, 2024
@darioillusorium
Copy link

Hello! Thank you for fixing this error. I wanted to ask if there is a temporary way to ignore this error until you finish fixing it. I am using a dropdown that works perfectly for my use but when changing the size of the window this error pops up on the screen. I just close it and everything and its ok. Is there a way to prevent the error from poping up on the screen temporarily?

Thanks you very much! :)

@mlmoravek
Copy link
Member Author

@darioillusorium Don't use Vue 3.4, use Vue 3.3.x. However, you can also use Oruga 0.7, which is based on the Options API and could work with the latest Vue version. As of 0.8, we have refactored the components to the Composition API, which contains this bug with Vue 3.4.
If you have any other problems, please open a separate issue.

@mlmoravek mlmoravek requested a review from jtommy February 1, 2024 17:14
@mlmoravek mlmoravek merged commit 0a7f9b4 into oruga-ui:develop Feb 1, 2024
8 of 9 checks passed
@mlmoravek mlmoravek deleted the bug/computed_class branch February 1, 2024 19:41
@l00k
Copy link

l00k commented Feb 2, 2024

When next release with this fix?

@mlmoravek
Copy link
Member Author

@l00k
I want to finish my a11y enhancement first (#723) - still working on it.
Then I will see which issues I can fix quickly.
And then I will do the new release 🙂

I don't want to make any date prediction, but I will try to do it this month - be aware that Oruga is a side project and nobody is working full time on it!

@l00k
Copy link

l00k commented Feb 2, 2024

I just thought fix (as independent thing) could be pushed without waiting for other bugfixes or improves.

My project which uses oruga is also side thing :) so.. yea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactoring This involves some refactoring of existing code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Got error "Error: useComputedClass must be called within a component setup function."
4 participants