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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docs to lib.rs + macros.rs. Simplify clone! #49

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pauldorehill
Copy link

@pauldorehill pauldorehill commented Aug 9, 2021

Hi @Pauan,

I've started adding some docs based on my experience of using dominator and also all the gold in the Discord that we get from asking you questions 馃槃 . I also updated the clone macro which may constitute a breaking change...

@iMplode-nZ
Copy link
Contributor

@Pauan these docs are very nice, could you mind merging it?

@evelant
Copy link

evelant commented Jan 2, 2022

These docs are very helpful! They would have helped me understand dominator a lot more easily and quickly. @Pauan anything blocking merging these?

@Pauan
Copy link
Owner

Pauan commented Jan 3, 2022

@AndrewMorsillo It's just a very large change, so I haven't been able to find the time to review it.

@derekdreery derekdreery mentioned this pull request Feb 13, 2022
@derekdreery
Copy link

@Pauan would it be OK if someone other than you reviewed it?

Copy link
Contributor

@TristanCacqueray TristanCacqueray left a comment

Choose a reason for hiding this comment

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

Hi @Pauan, first of all, thank you so much for dominator and rust-signals, they are quite a treat!

Would it help if such PR was broken into smaller pieces? For example:

  • Lib intro (a must have for the docs.rs/dominator landing page)
  • Macro docs
  • clone! refactor

//! `dominator` handles any necessary DOM updates on changes to the data. A basic app looks something like the
//! below, but refer to the [examples](https://github.com/Pauan/rust-dominator/tree/master/examples) for a
//! comprehensive set of best practices.
//! ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to ensure the examples compile, e.g. using:

Suggested change
//! ```
//! ```rust

or

Suggested change
//! ```
//! ```no_run

@pauldorehill
Copy link
Author

It was a long time since I submitted this, however I'm still using Dominator regularly. I could easily change it to one macro at a time etc.

@Pauan
Copy link
Owner

Pauan commented Oct 5, 2023

@TristanCacqueray Yes, small bite-sized pull requests are much much much much easier to review, since I don't need to devote a large chunk of time to reviewing it. Ideally a pull request per method / macro.

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.

None yet

6 participants