-
Notifications
You must be signed in to change notification settings - Fork 6
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(codex-ui): popup and confirm implementation #254
base: main
Are you sure you want to change the base?
Conversation
<style module> | ||
|
||
.popup { | ||
z-index: var(--z-popover); |
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.
lets a separate variable in z-axis.pcss
--z-popup
which will be cal(var(--z-popover) + 1)
/** | ||
* Text that will be displayed in the middle part of the confirm container | ||
*/ | ||
body: string; |
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.
body: string; | |
text: string; |
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.
Let's also create a useConfirm()
composable? It will be a preset using Popup + Confirm. It will simplify end usage.
Also, it is useful if "confirm" will return a promise.
Example:
const { confirm } = useConfirm()
async function deleteNote() {
try {
await confirm('Are you sure')?
service.deleteNote()
} catch(e){}
}
}); | ||
|
||
/** | ||
* Check every 100 ms to see if the user has pressed the button |
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.
you have callback in confirm props, so interval is not needed
onCancel, | ||
onConfirm, |
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.
redundant methods, they should be passed to the showPopup
import PageHeader from '../../components/PageHeader.vue'; | ||
import { Button, useConfirm } from '../../../src/vue'; | ||
|
||
const { confirm } = useConfirm(); |
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.
Let's create another page for Confirm examle. And Popup page will show anything else inside (for example just a stub text like "Popup can include any component") to demonstrate its nature
/** | ||
* Check if the user has pressed the confirm or close button | ||
*/ | ||
return new Promise<boolean>((resolve) => { |
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.
lets get rid of result
ref, you can wrap showPopup with this promise:
return new Promise<boolean>((resolve) => {
showPopup( ... )
})
box-shadow: inset 0 0 0 var(--delimiter-height) var(--base--border); | ||
} | ||
|
||
&__icon { |
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.
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.
also, please add hover to it. (Making icon and border color of --base--text
)
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.
onMounted(() => { | ||
document.addEventListener('keydown', closeOnEsc); | ||
}); | ||
|
||
onUnmounted(() => { | ||
document.removeEventListener('keydown', closeOnEsc); | ||
}); |
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.
move it to usePopup please
@@ -16,23 +16,15 @@ | |||
|
|||
<script setup lang="ts"> | |||
import PageHeader from '../../components/PageHeader.vue'; | |||
import { Button, useConfirm } from '../../../src/vue'; | |||
import { StubText, Button, usePopup } from '../../../src/vue'; |
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.
Stub Text should not be a part of the design system. You can define it here
Added
popup
andconfirm
componentsshowPopup()
functionshowPopup()
function can be added to any component, for example with @ clickpopup
itself using showPopup() withcomponent
andprops
parametersconfirm
component was embeded into thepopup
close button
confirm
buttons, you can trigger any events, passing them to theconfirm
ason-close-activate
andon-confirm-activate