Skip to content

Conversation

@larm-odoo
Copy link
Contributor

Adding new fleet documentation- this one is all the configurations in the config column.

@robodoo
Copy link
Collaborator

robodoo commented Sep 7, 2023

@larm-odoo larm-odoo marked this pull request as draft September 7, 2023 14:55
@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch 2 times, most recently from 96544f3 to 244b8b8 Compare September 11, 2023 15:00
@larm-odoo larm-odoo marked this pull request as ready for review September 11, 2023 15:01
@C3POdoo C3POdoo requested review from a team September 11, 2023 15:02
Copy link
Contributor

@jcs-odoo jcs-odoo left a comment

Choose a reason for hiding this comment

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

Hi @larm-odoo

If it fits the structure you have planned for this app's documentation, could you maybe move the configuration info to the app-level page (hr/fleet.rst)? Just a suggestion :)

Cheers :)

Comment on lines 1 to 3
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "configuration" only? it is in the "Fleet" category. But see also my suggestion in the main comment.

Copy link
Contributor Author

@larm-odoo larm-odoo Sep 11, 2023

Choose a reason for hiding this comment

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

Hi @jcs-odoo - I have two documents currently written in github for fleet, and I am working on a third. So ultimately, they will all be nested under 'fleet', so I didn't want to make the default fleet doc be this one.

I had done the format you suggested for other apps, where main doc I wrote covered 90% of things, but this time I was trying to chop the docs up into smaller digestible pieces, so that's why I tried to make separate docs for different items (addling a vehicle, configurations, reports, repairs, etc). If you don't think that's appropriate or a good idea, I can combine all the docs into one, but I was hoping to try and make the docs less overwhelming by having them separated out. Does that sound good?

I also know it means that once one doc is approved and posted, I will need to change the TOC so it looks correct with the newly added docs. I didn't want to wait to have the first doc be posted for review, have it go live, then add the rest, one at a time. That is why there are 2 (with another one on the way) posted now for review. Does that sound good/sound like a good plan? I have no objections in changing this method BTW- just thought it was a good thing to try.

As for the title- yes- I like that, and will update that now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @larm-odoo
You're right, it's better to have several pull requests rather than a massive one, at least in this documentation repository. Once the first PR is merged, you'll probably have a conflict on the other ones until you fix the toc tree. So that's fine :)

I agree this content only wouldn't make that much sense for the main page. Now that I see it again, I think my suggestion isn't better. "Configuration" alone is too vague (a bit like "misc." or "advanced"). Maybe "vehicle models and manufacturers"? That would be more representative. It's up to you :)

Have a good day

@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch from 244b8b8 to 916bc34 Compare September 11, 2023 16:09
Copy link
Contributor

@jero-odoo jero-odoo left a comment

Choose a reason for hiding this comment

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

@larm-odoo Nice work, just a few things to edit.

@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch from 916bc34 to 5fbb787 Compare September 20, 2023 15:45
@larm-odoo
Copy link
Contributor Author

Just FYI- this is done in 15.2 runbot. When requesting a new vehicle, in this doc, the word is CREATE but in 15,0 it is NEW- just an FYI so you know. That's really the only difference in the versions for this doc, so I wanted to point that out.

Copy link
Contributor

@tiku-odoo tiku-odoo left a comment

Choose a reason for hiding this comment

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

@larm-odoo

Good start...

I think this doc needs some more work though. I saw spelling errors after your team looked it over. I also have some suggestions. You repeat a section of this doc in the other doc (#5759) -- we should never be inputting info twice-- is it housed differently? can we prioritize and create an anchor?

The style seems cookie cut-- I would like to see better flow in these docs. Also keep in mind to write smart for 16/17. (save for example--- "if applicable, click save")

I would like to take a look at this doc again when you draft more changes.

Let me know if you have any questions.

Thanks,
Tim 👍

@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch from 5fbb787 to b4d5771 Compare October 11, 2023 21:01
@larm-odoo
Copy link
Contributor Author

Hi @tiku-odoo - I went through and addressed all your comments. I think everything should be clarified, but if not, let me know!
Ready for a second look.

Copy link
Contributor

@tiku-odoo tiku-odoo left a comment

Choose a reason for hiding this comment

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

@larm-odoo

Looks much better! A couple of comments. It looks good.

See comment: #5759 (review)

I will approve once you can clarify the duplicate info. I know it's present on both forms; just confused why it's input twice.

@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch from b4d5771 to 2529a3c Compare October 16, 2023 14:44
@larm-odoo
Copy link
Contributor Author

Hi @tiku-odoo - I addressed everything and added an anchor in this doc (which will be linked to the other doc, the new vehicle doc). This doc needs to be published first so that the following doc can reference this one. Right now the other doc has a code error because it is linking to an anchor in this doc, which doesn't exist yet online. Please check/approve this one first, so I can have this published, then I will get the other doc published. Any issues with this let me know- thank you!

@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch from 2529a3c to f9185bd Compare October 16, 2023 15:18
@larm-odoo larm-odoo requested a review from tiku-odoo October 16, 2023 15:25
Copy link
Contributor

@tiku-odoo tiku-odoo left a comment

Choose a reason for hiding this comment

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

Approving this content. Great job adding the anchor for the process you refer to in #5759

@larm-odoo larm-odoo requested a review from a team October 16, 2023 15:47
@larm-odoo
Copy link
Contributor Author

When this PR closes, get [ADD] fleet: add a new vehicle
#5759 merged next!

Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Hi @larm-odoo! Awesome job with the content in this PR, very thorough 👍

I do have a few suggestions after my technical review, which you can review below.

Please tag me for another look once my suggestions have been addressed, thank you!

@larm-odoo larm-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch 2 times, most recently from 2fa9f45 to 60b0ac2 Compare November 27, 2023 18:07
@larm-odoo
Copy link
Contributor Author

Thank you Sam for your help and review! This is ready for another look.

Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Hi @larm-odoo! Thanks for implementing my changes, I caught one small edit that I will push up after this review. Approving now, and moving forward. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line 🙂

Suggested change

@samueljlieber samueljlieber force-pushed the 15.0-fleet-new-configuration-doc-larm branch from 60b0ac2 to 1634347 Compare November 27, 2023 21:55
@samueljlieber
Copy link
Contributor

Hi @StraubCreative this PR is good to go 👍

Co-authored-by: Sam Lieber (sali) <36018073+samueljlieber@users.noreply.github.com>
@jero-odoo jero-odoo force-pushed the 15.0-fleet-new-configuration-doc-larm branch from 1634347 to fb21652 Compare December 21, 2023 17:04
Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

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.

7 participants