Skip to content

(fix) O3-4613 Removing previous sub properties from Form json after changing Question and Rendering Type #446

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

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

Conversation

yoursanonymous
Copy link
Contributor

@yoursanonymous yoursanonymous commented Apr 8, 2025

…roup button

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

When we create a question of type "encounterDateTime", we have radio buttons of "type of date picker to show" should be selected when we save a form but, it's not working.

Screenshots

Before

beforeSave.mp4

After

afterSave.mp4

Related Issue

https://openmrs.atlassian.net/browse/O3-4613

Other

@atulyadav745
Copy link
Contributor

Thanks @yoursanonymous, great catch! Really appreciate you pointing this out.

Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

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

I don't think its really necessary to default pick a datepicker type for this, but if we are doing it, its much better to set it here when the rendering type is being set rather than use an useEffect-

.
We should avoid the usage of useEffects wherever we can - https://react.dev/learn/you-might-not-need-an-effect

@yoursanonymous yoursanonymous changed the title O3-4613 Choosing a default radio button "type of date picker to show" radio group (fix) O3-4613 Choosing a default radio button "type of date picker to show" radio group Apr 9, 2025
@yoursanonymous
Copy link
Contributor Author

yoursanonymous commented Apr 9, 2025

Hi @NethmiRodrigo, Thank you for reviewing my PR,
Sorry just realized by looking at a similar 'ui-select-extended' component that using 'defaultSelected' property of RadioButtonGroup would be a better way to set the default value for Date picker type, made the required changes as well.

Could you please review it again.

@yoursanonymous
Copy link
Contributor Author

Hi @NethmiRodrigo, after trying out multiple scenarios the issue was found to be pretty big one in which when we change the rendering type, the subsequent subproperties of previous rendering type was not removed from the Form json and the default value selected for various fields in interactive builder was also not added automatically by default to json object.

@yoursanonymous
Copy link
Contributor Author

yoursanonymous commented Apr 28, 2025

before.o3-4613.mp4
after.o3-4613.mp4

@yoursanonymous
Copy link
Contributor Author

yoursanonymous commented Apr 28, 2025

Removing unnecesary fields during questionType change.

beforeQuestion1.mp4
afterQuestion.mp4

@yoursanonymous
Copy link
Contributor Author

Hi @NethmiRodrigo, awaiting your approval, Kindly review

@yoursanonymous yoursanonymous changed the title (fix) O3-4613 Choosing a default radio button "type of date picker to show" radio group (fix) O3-4613 Removing previous sub properties from Form json after changing Question and Rendering Type Apr 28, 2025
@NethmiRodrigo
Copy link
Collaborator

Hi @NethmiRodrigo, after trying out multiple scenarios the issue was found to be pretty big one in which when we change the rendering type, the subsequent subproperties of previous rendering type was not removed from the Form json and the default value selected for various fields in interactive builder was also not added automatically by default to json object.

@yoursanonymous The issue you've pivoted to address is already being worked on in PR #391, which has several comments and insights on how to fix it properly.
You have two good options moving forward:

  1. Revert this PR back to your original scope (addressing the initial ticket with the suggestions I provided in my first two reviews, which is sufficient to fix your initial problem, this current issue that you are addressing doesn't affect that)
  2. Collaborate on PR (fix) O3-4320: Remove Irrelevant Properties When Changing Question Type #391 if you're particularly interested in solving that specific issue

For future reference, it's best practice to keep PRs focused on their original scope. If you want to work on a different issue, please open a new PR rather than changing direction mid-review. This wastes reviewer time and disrupts our development process. One PR = one focused purpose.
I'm happy to help get your original work merged if you decide to go back to option 1.

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.

3 participants