-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add dialog demo #4
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
base: main
Are you sure you want to change the base?
Conversation
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 think this looks good overall. I few minor things, and I think I'd like a Miriam/Stacy review as well!
src/dialog/style.css
Outdated
margin: unset; | ||
text-align: center; | ||
text-wrap: balance; | ||
transition: |
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.
Could we also add a slight movement transition from the bottom? I think from bottom: -20px
to bottom: 0px
makes it feel natural.
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.
fixed in 8216c8a
src/dialog/index.html
Outdated
<article> | ||
<h1 data-heading="main"> Popping up with the <code>dialog</code> element</h1> | ||
<section> | ||
<h2 data-heading> Some Jumping Spiders |
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.
Could we do a play on JS = jumping spiders?
"Less JS (JavaScript), More JS (Jumping Spiders)"
"Vanilla JS (but the JS is Jumping spiders)"
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.
What about "Less JS, More Jumping Spiders"? 🕷️🕸️
✅ Deploy Preview for oddbaseline ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
src/dialog/style.css
Outdated
inherits: true; | ||
} | ||
|
||
dialog[open] { |
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 are several distinct dialog
& dialog[open]
blocks, even though we're using nesting for other things. This could get moved into the main dialog
style block and become &[open]
?
src/dialog/style.css
Outdated
transition: | ||
opacity var(--transition-timing-slower) ease-in, | ||
overlay var(--transition-timing-fast) linear, | ||
display var(--transition-timing-fast) linear, |
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.
the out animation is jumpy because display animates so much faster than opacity. Consider making display one of the slowest, so it doesn't disappear before it's done fading?
src/dialog/style.css
Outdated
} | ||
} | ||
|
||
dialog::backdrop { |
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 seems like it could be nested in the main block as &::backdrop
?
src/dialog/style.css
Outdated
dialog[open]::backdrop { | ||
--backdrop-opacity: 1; | ||
--backdrop: linear-gradient(140deg, | ||
var(--backdrop-gradient-start), | ||
var(--backdrop-gradient-end)); | ||
--filter: blur(2px); | ||
animation: show-gradient var(--transition-timing-slower) forwards; | ||
} | ||
|
||
@starting-style { | ||
dialog[open]::backdrop { | ||
--filter: none; | ||
--backdrop-opacity: 0; | ||
} | ||
} |
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.
dialog[open]::backdrop { | |
--backdrop-opacity: 1; | |
--backdrop: linear-gradient(140deg, | |
var(--backdrop-gradient-start), | |
var(--backdrop-gradient-end)); | |
--filter: blur(2px); | |
animation: show-gradient var(--transition-timing-slower) forwards; | |
} | |
@starting-style { | |
dialog[open]::backdrop { | |
--filter: none; | |
--backdrop-opacity: 0; | |
} | |
} | |
dialog[open]::backdrop { | |
--backdrop-opacity: 1; | |
--backdrop: linear-gradient(140deg, | |
var(--backdrop-gradient-start), | |
var(--backdrop-gradient-end)); | |
--filter: blur(2px); | |
animation: show-gradient var(--transition-timing-slower) forwards; | |
@starting-style { | |
--filter: none; | |
--backdrop-opacity: 0; | |
} | |
} |
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.
(just using nesting again)
src/dialog/style.css
Outdated
#privacy-popover:popover-open { | ||
--popover-background: var(--color-highlight); | ||
--popover-opacity: 1; | ||
--popoover-inset: 0; | ||
} | ||
|
||
@starting-style { | ||
#privacy-popover:popover-open { | ||
--popover-background: transparent; | ||
--popover-opacity: 0; | ||
--popoover-inset: -20px; | ||
} | ||
} | ||
|
||
#privacy-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.
These also seem like they could use nesting?
@mirisuzanne I think I addressed all of the nesting and animation issues, so this is ready for another quick look. |
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.
@dvdherron looks great. There's a little linting that I think would clean up the code some, but isn't essential and we've never discussed any 'rules for nesting' or anything. To me it reads better if the nested stuff can be ordered:
- properties
- variants (like
&[open]
) - descendants (like
[data-heading]
)
Since there's specificity differences at play, I think that can all be reordered safely without impacting the cascade at all.
@mirisuzanne thanks. cleaned this up in 4fb757a |
Change into the src/dialog directory
Run npx http-server .
Open http://localhost:8080/