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

Make step 3 in Introduction doc clearer for MVC users #112

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

brian-n-millar
Copy link

Q A
Documentation yes
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

This PR is in relation to my issue #111.
Updates the Introduction to give more clarity to step 3 for MVC users.
This is my first PR to this project so apologies if I've made any mistakes.

Signed-off-by: Brian N. Millar <114314688+brian-n-millar@users.noreply.github.com>
Signed-off-by: Brian N. Millar <114314688+brian-n-millar@users.noreply.github.com>
Signed-off-by: Brian N. Millar <114314688+brian-n-millar@users.noreply.github.com>
'factories' => [
MyNamespace\Command\MyCommand::class => MyNamespace\Command\MyCommandFactory::class,
],
],
];
```

> ### Container Config Identifier
Copy link
Member

Choose a reason for hiding this comment

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

You will get rid of the markdown issues by adding an empty line here.
Putting a newline here with > would already achieve that.

Copy link
Member

@froschdesign froschdesign May 9, 2023

Choose a reason for hiding this comment

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

@brian-n-millar
Unfortunately, the documentation is a bit misleading here because you do not need step 3. and 4. if you use a config provider class in your laminas-mvc module.
And this additional explanation does not really help here. There must be a clear separation for Mezzio, a laminas-mvc based application and others.

At the moment, unfortunately, I don't know what is meant by "components" either:

Integrating in Components

If you'd like to add integration with laminas-cli into your components…

https://docs.laminas.dev/laminas-cli/intro/#integrating-in-components


There are more problems on this page:

  • it is called "Introduction" but contains the usage
  • the installation belongs on its own page
  • the steps to create a command is incomplete and should be under its own heading
  • the basic usage should follow an example, including the call via the console

Copy link
Member

Choose a reason for hiding this comment

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

@froschdesign

I agree that the current documentation might be miss-structured but I think we should re-structure this in a dedicated issue.
Since the documentation does actually only provide details for mezzio, what would be your suggestion on how to split this into pieces without totally re-structuring the whole intro file?

@boesing boesing added Documentation Improvements or additions to documentation Enhancement and removed Enhancement labels May 8, 2023
@boesing boesing requested a review from froschdesign May 8, 2023 23:17
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM.
Requested review from @froschdesign as he tends to have decent feedback on documentation.

@gsteel
Copy link
Member

gsteel commented Jan 2, 2024

@froschdesign - Whilst there are further improvements to be made to the docs, I'm about to do a release and it looks to me like this is a sound improvement regardless of other noted flaws, so I'll merge and release. Hope that's OK 🤞

Thanks @brian-n-millar

@gsteel gsteel merged commit c9fa4df into laminas:1.9.x Jan 2, 2024
@gsteel gsteel added this to the 1.10.0 milestone Jan 2, 2024
@gsteel gsteel self-assigned this Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants