-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
[REF] l10n: PoC doc restructure for CO #3167
Conversation
93428cc
to
f6c29c3
Compare
Updates in f6c29c3:
PR passes CI Checks now— free to continue with layout rebuild and content updates 🚀 cc: @samueljlieber |
@odoo/accounting-doc-review this is still a work in progress but worth bringing up on your radar. cc: @jcs-odoo |
Hi guys, I started my review, but I guess this is not finished, so I'll wait for you to confirm it once it's done. :) Thanks! |
f6c29c3
to
4265818
Compare
Hi @StraubCreative, thank you for getting this PR up to date! Updates in 4265818:
|
56e0af8
to
8c56efd
Compare
56e0af8 to remove 16.0 specific images ➡️ 8c56efd to add 14.0 specific images. Hey there @StraubCreative, @vbe-odoo, & @ren-odoo 👋 can you please review the entire CO L10n content and add suggestions as necessary? Thank you everyone for your continued hard work on this! |
content/applications/finance/accounting/fiscal_localizations/localizations/colombia.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/accounting/fiscal_localizations/localizations/colombia.rst
Outdated
Show resolved
Hide resolved
I left some minor suggestions, but general structure considering the new approach, looks good :) There some images that are outdated, mainly because this localization has have several changes since the first documentation was created, there are well important new workflows and use cases that we want to add to the website documentation. Should we include this improvements and additions in this PR or should we handle this on a diferent PR once the new general structure is merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello :) just a few small comments after having scanned the pr, not in detail.
I wonder if splitting the page into several smaller ones works well here. Maybe it can, but it feels wrong as it is now...
The "Colombia" category page is empty and would only have a link to a youtube video. If there was some useful content redirecting to the subpages, then why not, but then it would probably become redundant with "workflow" and "reports".
Well, have a good weekend and... see you next year 😁
content/applications/finance/accounting/fiscal_localizations/localizations/colombia.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/accounting/fiscal_localizations/localizations/colombia.rst
Outdated
Show resolved
Hide resolved
...pplications/finance/accounting/fiscal_localizations/localizations/colombia/configuration.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/accounting/fiscal_localizations/localizations/colombia/reports.rst
Outdated
Show resolved
Hide resolved
8c56efd
to
f22a6e5
Compare
Thank you @jcs-odoo and @ren-odoo for the suggestions 🙂 Updates in f22a6e5:
I added the introduction description as a possible solution to @jcs-odoo comment, let me know what you all think. @ren-odoo regarding your comment about adding new workflows and use cases in this PR, I think it would be best to handle this in a different PR once these structural changes are merged - wdyt @StraubCreative? See you next year 🥳 |
f22a6e5
to
adcafb4
Compare
Updates in adcafb4:
|
adcafb4
to
1849105
Compare
Rebased to latest in 1849105 and reviewed the structure changes again. A handful of the images in this PR are a bit wider than they should be, but are already too heavily compressed to resize. Ideally we should retake the following:
|
Quick touchups on b478f96. Checks pass ✅ @jcs-odoo and @odoo/localizations-doc-review team, The purpose of this PR was to break apart the CO l10n documentation into individual RST files so the content was more digestible in steps for users. After working with @ren-odoo 's team, this was the way we thought it would best make sense right now, and for developing further on future PRs. If approved, we'd expand on these docs more for CO with more in-depth written instruction, and would duplicate the PoC structure to other LATAM l10n documentation (PE, AR, EC, etc.), as the opportunity arises. Thanks in advance for your review and thank you @samueljlieber and Team @ren-odoo for your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, and thanks again for all the great work!
I still think all the content could fit on a single page if there is a nice intro that introduces the various sections of the page.
If you have a look at the structure of all three pages, they all have a single h2 heading, which defeats the purpose of having one at all somehow. Also, the reports page is still ultra short. Combined into a single page, these headings become sections that look well organized.
I'll force-push this branch in a minute. I kept changes in separate commits so we can easily pick or drop any if needed. :) We'll squash them all into a single commit before merging the PR.
- 1st commit is the original one (I also rebased the branch since there was a bug with werkzeug on older bases)
- 2nd commit: some changes and fixes (most are commented in this review normally)
- 3rd commit: moving everything to a single page, to see how it would look like
- 4th commit: adding some custom anchors to headings to link them in the intro of the page, as suggested previously in an old comment (although I hoped at the time we could use cards) but it would certainly need to be improved. It's just to give you an example
I agree 100% that longer docs can be split into several ones, but here it still seems a bit overkill, IMO. Also, longer pages could sometimes have less content and go straight to the point, have links to other pages that already describe the same flow (I'm not saying this for this colombian doc, but in general), and have fewer images (for example the Mexican localization)
Anyway, let me know what you think of it. :) Maybe I don't see something.
Cheers!
content/applications/finance/fiscal_localizations/colombia/reports.rst
Outdated
Show resolved
Hide resolved
Modules installation | ||
-------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section won't be useful in more recent versions of the doc since the packages installation has been ultra simplified (and will be described in the fiscal_localizations parent page (cc @chiaraprattico @dade-odoo @toaa-odoo )
- Default :ref:`fiscal localization package <fiscal_localizations/packages>` | ||
* - :guilabel:`Colombia E-invoice Integration` | ||
- `l10n_co_edi` | ||
- Carvajal e-invoicing integration | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing the other modules that can be installed
content/applications/finance/fiscal_localizations/colombia/workflows.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/colombia/workflows.rst
Outdated
Show resolved
Hide resolved
b478f96
to
1f41de9
Compare
Hi @jcs-odoo 👋 thank you for the informative response and commits! I agree with your changes, and I have another PR #4425 that improves the content which I will now update to follow the structure you have implemented on this PR. I'm happy to squash commits 1, 2, 3 and 4 they look good to me 🙂 please let me know if you would like me to do so! |
1f41de9
to
a2a282c
Compare
a2a282c
to
a36a5c8
Compare
@robodoo r+ |
closes #3167 Signed-off-by: Jonathan Castillo (jcs) <jcs@odoo.com>
Cherry pick of #2659, now pointing to the correct base branch.
localizations
└── colombia
└── colombia.rst
..........├── configuration..........├── reports
..........├── workflows
..........├── configuration.rst..........├── reports.rst
..........└── workflows.rst
EDIT: summarized doc structure, removed
configuration
doc/folder, merged content intocolombia.rst
per the ideal User Doc Structure guidelines.