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

Forms section migration and consolidation #1696

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

esthermmoturi
Copy link
Contributor

@esthermmoturi esthermmoturi commented Nov 7, 2024

One of 2 PRs to close #1639

This PR is part of the docsite revamp work, the idea is to consolidate CHT Concepts into one section. In this case, the PR consolidated documentation on what CHT forms are , how to configure and maintain them.

All files will be placed under Building/Forms folder. Here are the items that will be moved:

Copy link
Member

@andrablaj andrablaj left a comment

Choose a reason for hiding this comment

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

@esthermmoturi I only had the time to review a few files. I will be off for a few days, so I will remove myself as a reviewer and suggest you add another team member. Sorry for that!

content/en/building/cht-forms/_index.md Outdated Show resolved Hide resolved
content/en/building/cht-forms/_index.md Outdated Show resolved Hide resolved
content/en/building/cht-forms/_index.md Outdated Show resolved Hide resolved
@esthermmoturi
Copy link
Contributor Author

Thank you for the suggestions @andrablaj . No worries. I will assign somebody else. Enjoy your time off!

Copy link
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

This PR nor the upstream ticket are not clear to an outside reviewer what the goal is. I think it's to move building/reference/forms/ to building/cht-forms/forms/ ? Please add more details to the ticket and the PR. Here's an example PR that has a video demonstrating the change (pretty involved!). A more simple approach would be to list out the changes in the PR. As well, the ticket should have enough details that folks know what the change is about.

I assume we a already decided on the building/cht-forms/forms path? I find it confusing and think we should consider changing it. This is because on the current site we have this hierarchy:

Building            (building)
    Reference       (building/reference)
        forms/      (building/reference/forms)
            app     (building/reference/forms/app)
            collect (building/reference/forms/collect)
            contact (building/reference/forms/contact)

In this PR as is, we want to do this:

Building            (building)
    Forms           (building/cht-forms)
        forms       (building/cht-forms/forms)
            app     (building/cht-forms/forms/app)
            collect (building/cht-forms/forms/collect)
            contact (building/cht-forms/forms/contact)

The use of the /cht-forms URL and directory seems superfluous (already in the forms directory, we don't need another forms directory) and confusing (are there other non-cht forms?) . I propose we flatten it. While we're in there we can rename both Form Properties and Versioning forms. This presents a very logical hierarchy without the repetitious use of forms:

Building            (building)
    Forms           (building/forms)
        app         (building/forms/app)
        collect     (building/forms/collect)
        contact     (building/forms/contact)
        Properties  (building/forms/properties)
        Versioning  (building/forms/versioning)

content/en/building/cht-forms/_index.md Outdated Show resolved Hide resolved
content/en/building/cht-forms/_index.md Outdated Show resolved Hide resolved
@esthermmoturi
Copy link
Contributor Author

I have left the .md files named as is(form-properties.md and versioning.md) but changed the Title that appears in the site. This is what we expect to see:
image

@mrjones-plip
Copy link
Contributor

@esthermmoturi - I'll try and get to this in the next couple of days!

Copy link
Contributor

@mrjones-plip mrjones-plip 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 doing a round of fixes! I think maybe one change got reverted? I unresolved it for you to follow up.

Otherwise two other changes please:

  1. while this URL is being created let's move it from /building/cht-forms/ -> /building/forms/ . One less CHT is better!

  2. there's a number of 404s this PR creates. Please use the checker script to itterate and ensure they're all fixed:

http://localhost:1313/building/guides/forms/additional-docs/ 404
http://localhost:1313/building/guides/forms/app-form-sms/ 404
http://localhost:1313/building/guides/forms/form-inputs/ 404
http://localhost:1313/building/guides/forms/google-drive/ 404
http://localhost:1313/building/guides/forms/multimedia/ 404
http://localhost:1313/building/guides/forms/report-titles/ 404
http://localhost:1313/building/guides/forms/uhc-mode/ 404
http://localhost:1313/building/guides/forms/wealth-quintiles/ 404
http://localhost:1313/building/reference/forms/ 404
http://localhost:1313/building/reference/forms/app/ 404
http://localhost:1313/building/reference/forms/collect/ 404
http://localhost:1313/building/reference/forms/contact/ 404

content/en/building/cht-forms/_index.md Outdated Show resolved Hide resolved
@esthermmoturi
Copy link
Contributor Author

esthermmoturi commented Nov 15, 2024

The script is returning 200 as follows:

http://localhost:1313/building/guides/forms/additional-docs/ 200
http://localhost:1313/building/guides/forms/app-form-sms/ 200
http://localhost:1313/building/guides/forms/form-inputs/ 200
http://localhost:1313/building/guides/forms/google-drive/ 200
http://localhost:1313/building/guides/forms/multimedia/ 200
http://localhost:1313/building/guides/forms/report-titles/ 200
http://localhost:1313/building/guides/forms/uhc-mode/ 200
http://localhost:1313/building/guides/forms/versioning/ 200
http://localhost:1313/building/guides/forms/ 200
http://localhost:1313/building/guides/forms/wealth-quintiles/ 200

Copy link
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

Can you have another look at the 404s? Between the time I reviewed this PR and now, you made two commits (1 and 2) and neither has any additions to aliases: section. I'm seeing all the 404s I found as still needing to be fixed.

As well, while you're in there, can you move /building/forms/configuring-forms/ to be just /building/forms/configuring/ ? I should have noticed this from the start, but the number of forms in the URL seems overkill (we don't need to fix the existing URLs, only the new ones I think!):

/building/forms/configuring-forms/form-inputs/

@esthermmoturi
Copy link
Contributor Author

esthermmoturi commented Nov 19, 2024

The standard naming for all other sections is configuring-xxx(example here ), this makes it easier to identify files other than each section having a file named configuring so let's leave it as is.

Aliases were not added since the files had not been merged to main with new names/links.(for commit 1) - commit 2 omitted a line within the form. I pushed commits to address feedback provided within the PR.

Let me set up a quick 15 min call to look through the 404s because I am still getting 200s. I think I am missing something.

@esthermmoturi
Copy link
Contributor Author

Thanks @mrjones-plip for the help. I have fixed the 404s

@mrjones-plip
Copy link
Contributor

yay! 404s all look good!

If we can tend to the extra forms in the URL, I think we'll be good:

As well, while you're in there, can you move /building/forms/configuring-forms/ to be just /building/forms/configuring/ ? I should have noticed this from the start, but the number of forms in the URL seems overkill (we don't need to fix the existing URLs, only the new ones I think!):

/building/forms/configuring-forms/form-inputs/

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.

Move forms section
3 participants