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

Add prompt function to the dialogue service. #1350

Merged
merged 5 commits into from
Jul 4, 2020

Conversation

teymour-aldridge
Copy link
Contributor

  • Improve the clarity of some doc comments
  • Fix a typo in a map_err

Description

Checklist:

  • I have run ./ci/run_stable_checks.sh
  • I have reviewed my own code
  • [ ] I have added tests doesn't apply

* Improve the clarity of some doc comments
* Fix a typo in a `map_err`
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Thanks for adding this and fixing up the typos!

pub fn prompt(
message: &str,
#[cfg(feature = "web_sys")] default: Option<&str>,
#[cfg(feature = "std_web")] _: Option<&str>,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this argument on std_web?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't, but it gets messy if functions have different signatures IMO.

Copy link
Member

Choose a reason for hiding this comment

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

We generate separate docs for stdweb so I think it should be ok, do you have other concerns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I'll remove the argument on stdweb (+ add a note in the docstring).

Copy link
Member

@jstarry jstarry Jul 4, 2020

Choose a reason for hiding this comment

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

Oh, I didn't think about the doc string! You can check out yew/src/lib.rs for example on how to render conditional docs

@@ -41,4 +44,45 @@ impl DialogService {
feature = "web_sys" => utils::window().confirm_with_message(message).unwrap(),
}
}

/// Prompts the user to input a message. In most browsers this will open an alert box with
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a note about what this function returns. For example, when does it return None? I think we might be able to add a test for that case btw

Copy link
Contributor Author

@teymour-aldridge teymour-aldridge Jul 4, 2020

Choose a reason for hiding this comment

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

Ooh, that's a good idea. I want to look into developing a set of integration tests (ideally from the examples) as well so that we can test things which can only be tested in a browser.

@jstarry jstarry merged commit 151b5b7 into yewstack:master Jul 4, 2020
jstarry pushed a commit that referenced this pull request Aug 16, 2020
* Add `prompt` function to the dialogue service.
* Improve the clarity of some doc comments
* Fix a typo in a `map_err`

* Fix `cfg_match!` usage.

* Add `stdweb` support.

* Fix the `prompt` method.

* Add different docs for `stdweb` and `web_sys`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants