-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(useZIndex): new composable #156
Conversation
✅ Deploy Preview for anu-vue ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
The auto-incrementing z-index is a popular solution, and it has been implemented in libraries such as naive-ui and element-plus. |
oh, Then I guess saying "anything you think of might be already implemented by someone somewhere" is true 🤣 |
@IcetCode were you getting this linting errors in test? |
I'm also being bugged by this lint error. |
fix #34