Skip to content

Example HTML Dialog #38325

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

Closed
NPBogdan opened this issue Feb 25, 2024 · 6 comments · Fixed by #38359
Closed

Example HTML Dialog #38325

NPBogdan opened this issue Feb 25, 2024 · 6 comments · Fixed by #38359
Assignees
Labels
accepting PR Feel free to open a PR to resolve this issue Content:WebAPI Web API docs goal: clarity (Experimental label) Issues about unclear/confusing/inconcise content.

Comments

@NPBogdan
Copy link

What information was incorrect, unhelpful, or incomplete?

I believe it's not working as expected. The part where you handleUserInput(). I believe something is not correct.

What did you expect to see?

I expect to see a selected option in the output div, but it shows the btn value. I hope i'm not wrong. I understand the code but i think you wanted to show a selected option.

Do you have any supporting links, references, or citations?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/returnValue#examples

Do you have anything more you want to share?

Nope.

@NPBogdan NPBogdan added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Feb 25, 2024
Copy link
Contributor

It looks like this is your first issue. Welcome! 👋 One of the project maintainers will be with you as soon as possible. We appreciate your patience. To safeguard the health of the project, please take a moment to read our code of conduct.

@caugner caugner added the goal: clarity (Experimental label) Issues about unclear/confusing/inconcise content. label Mar 15, 2024
@bsmth
Copy link
Member

bsmth commented Apr 9, 2024

Thanks a lot for opening this one. I took a look through this and I agree it's a little hard to tell what the intention is. What do you think about reducing it down to something like:

<dialog id="termsDialog">
  <p>Do you agree to the Terms of Service(link)?</p>
  <button id="declineButton" value="declined">Decline</button>
  <button id="acceptButton" value="accepted">Accept</button>
</dialog>

<p>
  <button id="openDialog">Review ToS</button>
</p>
<p id="statusText"></p>

And in the JS we have something like:

  // setup etc.
  
  termsDialog.addEventListener('close', () => {
    statusText.innerText = 'ToS dialog return value: ' + termsDialog.returnValue;
  });

@bsmth bsmth self-assigned this Apr 9, 2024
@bsmth bsmth added awaiting response Awaiting for author to address review/feedback and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Apr 9, 2024
@low-perry
Copy link
Contributor

low-perry commented Jun 15, 2024

Hello, I was reading through this issue and noticed the following sentences:

I expect to see a selected option in the output div, but it shows the btn value. I hope i'm not wrong. I understand the code but i think you wanted to show a selected option.

I think the intent of the author of the docs is to show, how the returnValue property of the <dialog> element works.
It states that this property "gets or sets the return value for the dialog, usually to indicate which button the user pressed to close it". And if that's the case, the intent was never to show which option the user chose from the dropdown, but what button closed the dialog.

@Josh-Cena Josh-Cena transferred this issue from mdn/interactive-examples Feb 25, 2025
@github-actions github-actions bot added the Content:WebAPI Web API docs label Feb 25, 2025
@Josh-Cena Josh-Cena added accepting PR Feel free to open a PR to resolve this issue and removed awaiting response Awaiting for author to address review/feedback labels Feb 25, 2025
@Josh-Cena
Copy link
Member

Accepting a PR along @bsmth's suggestion

@bsmth
Copy link
Member

bsmth commented Feb 26, 2025

the intent was never to show which option the user chose from the dropdown, but what button closed the dialog.

I agree with this, by the way. We should make that clear in the prose before the example. Either it's fixed in the prose or we update it to the suggestion above

@low-perry
Copy link
Contributor

low-perry commented Feb 27, 2025

I think the definition also needs to be rewritten.
The returnValue property of the <dialog> element is a string that stores the value provided when the dialog is closed using the .close(returnValue) method.

(I am struggling to come up with a better definition cause you can also set the returnValue just by passing a string to dialog.returnValue = "some string"

The HTML suggestion above (@bsmth ) is better than the original one.
I think the following js would be sufficient:

   const dialog = document.getElementById("termsDialog");
    const openDialog = document.getElementById("openDialog");
    const statusText = document.getElementById("statusText");
    const declineButton = document.getElementById("declineButton");
    const acceptButton = document.getElementById("acceptButton");

    function openCheck(dialog) {
      if (dialog.open) {
        statusText.innerText = "Dialog open";
      } else {
        statusText.innerText = "Dialog closed";
      }
    }

    function handleUserInput(returnValue) {
      if (!returnValue) {
        statusText.innerText += ". There was no return value";
      } else {
        statusText.innerText += ". Return value: " + returnValue;
      }
    }

    openDialog.addEventListener("click", () => {
      dialog.showModal();
      openCheck(dialog);
      handleUserInput(dialog.returnValue);
    });

    declineButton.addEventListener("click", () => {
      dialog.close("declined");
      openCheck(dialog);
      handleUserInput(dialog.returnValue);
    });

    acceptButton.addEventListener("click", () => {
      dialog.close("accepted");
      openCheck(dialog);
      handleUserInput(dialog.returnValue);
    });

    dialog.addEventListener("close", () => {
      openCheck(dialog);
      handleUserInput(dialog.returnValue);
    });

bsmth added a commit that referenced this issue Feb 28, 2025
* Update definition of `returnValue` of the <dialog> element

* Change example for clarity

* Fix inline html element `<kbd>`

* Format Example paragraph. Fix HTML identation.

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fix identation in HTML/JS example

* Remove unnecessary method , Combine the functions that close the dialog into a single one.

These changes are based on the comments left on my proposed changes:
1. [Remove `openCheck` method](https://github.com/mdn/content/pull/38359/files/cdea48036687a54cde93f5b6d119a3a7e252cc1b#r1975216037)
2. [Combine the functions](https://github.com/mdn/content/pull/38359/files/cdea48036687a54cde93f5b6d119a3a7e252cc1b#r1975219487)

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Brian Smith <[email protected]>
cssinate pushed a commit to cssinate/content that referenced this issue Apr 11, 2025
* Update definition of `returnValue` of the <dialog> element

* Change example for clarity

* Fix inline html element `<kbd>`

* Format Example paragraph. Fix HTML identation.

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fix identation in HTML/JS example

* Remove unnecessary method , Combine the functions that close the dialog into a single one.

These changes are based on the comments left on my proposed changes:
1. [Remove `openCheck` method](https://github.com/mdn/content/pull/38359/files/cdea48036687a54cde93f5b6d119a3a7e252cc1b#r1975216037)
2. [Combine the functions](https://github.com/mdn/content/pull/38359/files/cdea48036687a54cde93f5b6d119a3a7e252cc1b#r1975219487)

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

Co-authored-by: Brian Smith <[email protected]>

* Update files/en-us/web/api/htmldialogelement/returnvalue/index.md

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Brian Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting PR Feel free to open a PR to resolve this issue Content:WebAPI Web API docs goal: clarity (Experimental label) Issues about unclear/confusing/inconcise content.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@caugner @bsmth @NPBogdan @Josh-Cena @low-perry and others