Skip to content

feat(config): global config support #165

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
Closed
wants to merge 3 commits into from
Closed

feat(config): global config support #165

wants to merge 3 commits into from

Conversation

IcetCode
Copy link
Contributor

We can now configure the zIndex property in the following two ways:

  • app.use(anu)
// main.ts
app.use(anu, { zIndex:3000 })
  • use ConfigProvider component
// global
<AConfigProvider :zIndex="zIndex">
  <RouterView/>
<AConfigProvider/>

const zIndex = ref(3000)

// component scope
<AConfigProvider zIndex="2000">
   <ABtn>
     <ATooltip />
   </ABtn>
<AConfigProvider />

@netlify
Copy link

netlify bot commented Apr 19, 2023 •
edited
Loading

✅ Deploy Preview for anu-vue ready!

Name Link
🔨 Latest commit 625d7ef
🔍 Latest deploy log https://app.netlify.com/sites/anu-vue/deploys/643fd775cb1e400008c7a5fb
😎 Deploy Preview https://deploy-preview-165--anu-vue.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 settings.

@jd-solanki
Copy link
Owner

Hi 👋🏻

I noticed this PR has changes from another PR #156

Is it possible to update the PR so that I can review only relevant changes?

Awaiting your response.

@IcetCode
Copy link
Contributor Author

You're saying to update PR #156 to get the zIndex issue out of this PR, right?

@jd-solanki
Copy link
Owner

#156 looks perfectly fine, I will review it and merge it. The issue is changes of that PR are included in this PR as well.

Can we update this PR to remove code of #156? or it isn't possible?

@IcetCode
Copy link
Contributor Author

I'm not sure if that's possible. It seems like we have two choices now: either we abandon #156 and only merge this PR, or we merge #156 first and then merge the changes into this one.

@jd-solanki
Copy link
Owner

Alright, I will manage while reviewing both PR 👍🏻

@jd-solanki
Copy link
Owner

jd-solanki commented May 12, 2023 •
edited
Loading

Hey, I checked this PR and it looks really nice. I really appreciate the efforts you put into it.

I have something thoughts to share and we might have to discuss a few things before merging this PR. I hope that won't take much of your time.

To make sure we are on the same page, I want to let you know that the global config support we are trying to add in anu is similar to vuetify 3's global config. This way users can add global defaults for components like settings default variant for the alert to filled globally so users don't have to pass variant="filled" everytime. Additionally, this feature should also allow the user to override prop default value at runtime. Let's say for the group of 5 menu component we want to open them by hover then we can wrap config provider and set the default for that scope:

<AConfig :defaults={ menu: { trigger: 'hover' } }>
  <!-- menu component usage -->
</AConfig>

@IcetCode
Copy link
Contributor Author

This is great, I tried the config-provider component provided by Vuetify, such global configuration is really convenient for developers.

@jd-solanki
Copy link
Owner

I'm out of the town atm but I will update the PR as soon as I'll be able to work on it.

jd-solanki
jd-solanki reviewed Jun 12, 2023
View reviewed changes
packages/anu-vue/src/composables/useConfig.ts
Comment on lines +31 to +33
provideFn(configProviderContextKey, context)

provideFn(zIndexContextKey, computed(() => context.value.zIndex))
Copy link
Owner

Choose a reason for hiding this comment

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

Hey 👋🏻

While I was working on AConfig (same as AConfigProvider but also for component props) I had issue where if I update the config it also get updated upwards in DOM tree due to reactivity. Then I asked antfu & he suggested me directly passing new ref in provide instead of updating the value. While I was reviewing this PR for new release, I notices you are doing the same thing to avoid updating config in DOM upwards of AConfigProvider, Am I right?

excalidraw graph: props-defaults-explanation.excalidraw.zip

Copy link
Owner

@jd-solanki jd-solanki Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried the trick and it seems its working 🤯

I'll merge AConfigProvider & AConfig in this PR if possible!

@jd-solanki jd-solanki mentioned this pull request Jun 12, 2023
Closed
@jd-solanki
Copy link
Owner

closing in favor of #184 & #156

@jd-solanki jd-solanki closed this Jun 13, 2023
@IcetCode IcetCode deleted the feat/global-config branch June 13, 2023 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jd-solanki jd-solanki jd-solanki left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IcetCode @jd-solanki