Closed guastallaigor closed 5 years ago
I think the following demo is good 👍 Because it's a dialog including the style of backdrop.
<button type="button" class="nes-btn" onclick="document.getElementById('default-dialog').showModal();">Open dialog</button>
<dialog class="nes-dialog" id="default-dialog">
<form method="dialog">
<p class="title">Dialog</p>
<p>Alert: this is a dialog.</p>
<menu class="modal-menu">
<button class="nes-btn">Cancel</button>
<button class="nes-btn is-primary">Confirm</button>
</menu>
</form>
</dialog>
.modal-menu {
padding: 0;
text-align: right; /* or center */
}
And if we add an overlay just for the demo, people will open bug issues because they're using dialogs and it will not show any overlay, it's going to be confusing.
Could you let me know more about this? 🙏
Could you let me know more about this?
My mistake, for some reason the overlay wasn't showing up in my very old notebook, maybe due to the browser version or OS. But I've tested your demo code in my PC and your demo looks better :+1:
What should I do?
Also, should we add 4 buttons (or 4 checkboxes) to show the diversity of the dialogs (is-dark
, is-rounded
) or just one is fine?
I'll fix it and push it to this branch🛠
should we add 4 buttons (or 4 checkboxes) to show the diversity of the dialogs (
is-dark
,is-rounded
) or just one is fine?
It's better to have 4 buttons.
@guastallaigor I fixed it 🛠 Could you review this changes? 🙏
@guastallaigor I fixed it Could you review this changes?
Thank you for changing it @BcRikko. LGTM, but if I may give a suggestion, adding this would save more "screen space":
#dialogs > section {
display: inline-block;
}
Also, we can check this is another issue/PR, but there isn't a white border in the dialog (ref #281) when it's opened:
index.html
storybook
But it is showing up right in the storybook. I'm checking it but I haven't found the cause of this. Anyway, this is great and good to merge, thank you.
Oh my gosh 😇 Browsers other than Chrome cannot use dialog element. 😭 https://caniuse.com/#feat=dialog
The first suggestion you made is good.
@guastallaigor I fixed bug of dark dialog border. 🛠 and I added dialog polyfill for other than Chrome 😇
Oh my gosh Browsers other than Chrome cannot use dialog element. https://caniuse.com/#feat=dialog
The first suggestion you made is good.
Make sense then, I didn't thought about that. Because when I first test it, I used firefox on my notebook.
I think everything is good to merge for now, since it's a demo :+1: You can merge it when you can.
And in my opinion, after this PR we should be good to release version 1.1.0 :rocket:
:tada: This PR is included in version 2.0.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
ref #276.
Description This PR adds
Texts
andDialogs
toindex.html
(docs).Compatibility N/A
Caveats
For now I think it's better to just add some CSS to the
Dialog
, because I was thinking if we put a button that opens a dialog in the demo, without any overlay, it will look weird.And if we add an overlay just for the demo, people will open bug issues because they're using dialogs and it will not show any overlay, it's going to be confusing.
I've also changed from
Table
toTables
(just the word at the demo, and ids), because other components are pluralized and this one wasn't.Also found one bug regarding the
Dialog
component, but I will open another issues for that.