Skip to content
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

Adds support for HTML dialog object #171

Merged
merged 14 commits into from
Dec 13, 2018
Merged

Adds support for HTML dialog object #171

merged 14 commits into from
Dec 13, 2018

Conversation

amity
Copy link
Contributor

@amity amity commented Dec 11, 2018

Creates formatting for HTML dialog objects; modeled off of the current design for containers.  Created a test storybook container as well.
Currently doesn't support many options; hoping to add that later if I get the time.


HTMLのdialog、今のcontainerのデザインのようにします。storybook の例も作りました。
今のは、ちょっと簡単ですが… 時間があれば、私は後でもっと進めるつもりです。

<日本語があまり使いませんので、よくなければごめんなさい。>

screenshot 2018-12-11 11 12 17

soph-iest and others added 2 commits December 11, 2018 11:08
@amity
Copy link
Contributor Author

amity commented Dec 11, 2018

Related: I was consistently getting "build-storybook" not found errors (it's called in the base "build" script) until I added that line to the package.json. Please remove it if it's not necessary, but it seemed to just be a simple omission from what I could tell.


package.jsonの変えた点は、いつも”build-storybookのスクリプトがない”のエラーメッセージがいつも出ていました。("build"のスクリプトの終わりに呼ばれています。)だから、そのスクリプトを付け足しました。それはだめだったら、どうぞ決してください。

@amity amity changed the base branch from master to develop December 11, 2018 16:41
@trezy
Copy link
Member

trezy commented Dec 11, 2018

Hey @soph-iest, it looks like you already switched this PR to point to the develop branch instead of the master branch. Can you make sure to pull the latest develop down locally and merge it into your branch? It looks like your branch is missing some commits from our latest develop.

Once you've taken care of that we'll pull it down and review. 😊

@amity
Copy link
Contributor Author

amity commented Dec 11, 2018

Think that should've done it!

Copy link
Member

@trezy trezy left a comment

Choose a reason for hiding this comment

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

@soph-iest: Could you also remove css/nes.min.css from the PR? We've removed compiled files from our develop branch. Otherwise, this looks great! Once you address these items I'm excited to get it merged. 😁

package.json Outdated Show resolved Hide resolved
scss/elements/dialogs.scss Outdated Show resolved Hide resolved
@trezy trezy requested a review from a team December 12, 2018 01:01
@trezy trezy added the enhancement New feature or request label Dec 12, 2018
Copy link
Member

@BcRikko BcRikko left a comment

Choose a reason for hiding this comment

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

I have not tried it yet, but I think it's better to have a backdrop style 🤔
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

and Thank you for writing in Japanese 😆

scss/elements/dialogs.scss Show resolved Hide resolved
scss/elements/dialogs.scss Outdated Show resolved Hide resolved
@BcRikko
Copy link
Member

BcRikko commented Dec 12, 2018

Sorry, The backdrop style was provided by user agent stylesheet 😇
I didn't notice that style does not apply with <dialog open />. 🙈

Style has been applied when using dialogElement.showModal() in JavaScript

sample

<button type="button" class="nes-btn is-primary">open</button>
<dialog class="nes-dialog">
  <p>Hello world!</p>
</dialog>
document.querySelector('.nes-btn').addEventListener('click', () => {
  document.querySelector('.nes-dialog').showModal();
})

dialog

@amity
Copy link
Contributor Author

amity commented Dec 13, 2018

@BcRikko I was trying to consider the use case in which the user opens the dialog without using showModal() -- though that might be a standard approach to displaying dialogs, my thinking was to prioritize the functionality in all cases.
I've gone ahead and switched it to box-shadow for now.

@trezy
Copy link
Member

trezy commented Dec 13, 2018

@BcRikko Yeah, the HTML5 <dialog /> element has two different methods for being displayed.

  • dialog.show()
    This opens the <dialog /> in dialog mode, which means that it should not prevent the user from interacting with the other visible parts of the interface.
  • dialog.showModal()
    This opens the <dialog /> in modal mode, which means that it should prevent the user from interacting with the other visible parts of the interface.

That said, it may not be the worst idea to add an explicit style for the backdrop. Something like this would be great, @soph-iest:

.nes-modal {
  > .backdrop,
  &::backdrop { ... }
}

Copy link
Member

@trezy trezy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@trezy
Copy link
Member

trezy commented Dec 13, 2018

Thanks so much for the work, @soph-iest! 😁

@trezy trezy merged commit 5ae06e2 into nostalgic-css:develop Dec 13, 2018
@BcRikko
Copy link
Member

BcRikko commented Dec 18, 2018

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants