Skip to content

feat(useZIndex): new composable #156

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

Merged
merged 5 commits into from
Jun 13, 2023

Conversation

IcetCode
Copy link
Contributor

fix #34

@netlify
Copy link

netlify bot commented Apr 11, 2023

Deploy Preview for anu-vue ready!

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

What a coincident, I also had the same thought on implementation 🤯

Is this the common pattern in other framework as well or just two of us 🤔

export const zIndexContextKey: InjectionKey<Ref<number | undefined>> = Symbol('zIndexContextKey')

const zIndexCounter = ref(0)
export const defaultBaseZIndex = 2000 as const
Copy link
Owner

Choose a reason for hiding this comment

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

How about, If we pass this as config to useAnu with a default value of 2000 as per your code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a pretty good idea, but when we consider potentially needing more global-level configurations later on, wouldn't this make the useAnu function somewhat overly complex?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes it can but I had a thought that what will be best where we allow users to configure z-index then?

ATM, we are configuring themes in useAnu initially here: https://github.com/jd-solanki/anu/blob/main/packages/anu-vue/src/plugin.ts#L77-L80

Later, users can use useAnu composable to get the configured and runtime options like theme (only ATM).

Maybe we can move theme only composable to seperate composable like vuetify, useTheme. (I don't have implementation idea yet)

What's your thoughts?

Copy link
Owner

@jd-solanki jd-solanki Apr 13, 2023

Choose a reason for hiding this comment

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

P.S. In future, we will provide defaults provider like el-config-provider in future.

Are you on discord?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the Discord app? My username is Icet#3793.

I'm also pondering which approach to choose, and it seems there are two viable options at the moment:

  • ConfigProvider
<template>
  <a-config-provider :zIndex="zIndex">
    <app />
  </a-config-provider>
</template>

<script setup>
const zIndex = ref(1000)
<script/>
  • useAnuConfig
<template>
   <app />
</template>

<script setup>
const zIndex = ref(1000)
useAnuConfig({ zIndex })
<script/>

We may need more time to make a trade-off.

Copy link
Owner

Choose a reason for hiding this comment

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

I have pushed new commit for useAnu improvement: 0bc880d

Now, It might be clear, What I was saying. We will create useZIndex via createGlobalState composable like this and initialize it for the first time via user config like this. To configure, base z-index user can just pass it as config property like this.

This will allow us to create as many composable as we want with clear seperate of what each composable do.

For example, we already have useZIndex thanks to you. We can also convert useAnu composable to useTheme that solely handles theme related state.

According to me this is the best possible way to configure Anu now.

Overview:

  • Pass user config via app.use(anu, { ... })
  • We initialize as many composable as we want for clear seperation
  • (Additionally) we will be able to use this composable in features like defaults provider later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for taking a while to reply.

I think this method is great. Using configuration data at the global scope level gives us a lot of flexibility in providing more configuration options. It also solves the problem I was having about how to handle priority when there is a nested provider.

Copy link
Owner

Choose a reason for hiding this comment

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

Hey @IcetCode

Sorry I was busy lately. Just wanted to know, Are you going to update the PR or I should go ahead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @jd-solanki

I've been swamped with work too. I'll update the PR tomorrow for app.use(anu, { zIndex }) and config-provider. What do you think about this?

Copy link
Owner

Choose a reason for hiding this comment

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

No worries, I had time so I thought I can work on it if you don't have time.

I will wait for the update 😇

Thanks

@IcetCode
Copy link
Contributor Author

What a coincident, I also had the same thought on implementation 🤯

Is this the common pattern in other framework as well or just two of us 🤔

The auto-incrementing z-index is a popular solution, and it has been implemented in libraries such as naive-ui and element-plus.

@jd-solanki
Copy link
Owner

The auto-incrementing z-index is a popular solution

oh, Then I guess saying "anything you think of might be already implemented by someone somewhere" is true 🤣

@jd-solanki jd-solanki changed the title fix(floating): zIndex overlapping issue feat(useZIndex): new composable Jun 13, 2023
@jd-solanki jd-solanki merged commit 975121d into jd-solanki:main Jun 13, 2023
@jd-solanki
Copy link
Owner

@IcetCode were you getting this linting errors in test?

image

@IcetCode
Copy link
Contributor Author

I'm also being bugged by this lint error.

@IcetCode IcetCode deleted the fix(floating)/zIndex-issue branch June 20, 2023 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASelect options inside ADrawer are not visible
2 participants