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

Add Specification for GPHL Json Schema form #815

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Conversation

jbflo
Copy link
Member

@jbflo jbflo commented Nov 7, 2023

Update for Current form query_pre_strategy_params
renames ui filed keys for better display of form

@jbflo jbflo requested a review from rhfogh November 7, 2023 03:30
Copy link

github-actions bot commented Nov 7, 2023

Coverage

Coverage Report •
FileStmtsMissCoverMissing
mxcubecore/HardwareObjects/Gphl
   GphlWorkflow.py127212720%4–3032
TOTAL60524562077% 

Tests Skipped Failures Errors Time
1921 0 💤 0 ❌ 0 🔥 1m 36s ⏱️

@rhfogh
Copy link
Collaborator

rhfogh commented Nov 7, 2023

Looking good, but a quick discussion needed.

Two changes here, near as I can see:

  • change readonly to readOnly . Fine. I can do that in the mxcubeqt part

  • Get rid of definitions in schema, and replace with enum tag. No objection - the definitions are complex and tricky. Just one question: Doe the web interface make use of the value_dict? Currently all the pulldowns have the displayed label and the underlying value the same, but we might want to have those differ in some case.

Anyway, once we agree on this one, do you want me to make a new PR where I expand this to make the same changes in query_collection_strategy as in query_pre_strategy_params, and rebase it on the current tip (and make sure I adapt the qt version to it, which I need to do anyway). If you prefer I can leave it to you, of course.

@jbflo
Copy link
Member Author

jbflo commented Nov 7, 2023

  • Yes the UI library does not recognize readonly but readOnly

  • Again value_dict break the UI I read the documentation and it seen value_dict is not a valid key. so enum is to be use.

Hope they are not a big breaking to the qt.

I also will need to add some Changes to the uiSchema ditct cause it does not work in the Web UI .

You can Please push your Changes to this Branch if you like , so won't need to create another PR.
If they are Related to the Schemas and UISchema.

@rhfogh
Copy link
Collaborator

rhfogh commented Nov 7, 2023

OK, without value_dict there is no way of setting a pulldown where the visible labels are different from the values returned. So far we are not using that, so it is not too big a deal. I guess that means we should simply get rid of value_dict, which we can do, and find another way of transforming from the visible label to a desired value if we ever need it. The UI schema will be more interesting - here there might well be harder to find a way to get the kind of control over the generated UI that we have currently in Qt.

I have no pending changes, but there is a recent merge in mxcubecore that means that the jb-rasmus-gphl branch will need to be rebased on develop before the PR can be merged. I do not think there should be any clashes, but there are some changes in GphlWorkflow not too far away from yours. Anyway, I leave that to you.

Copy link
Collaborator

@rhfogh rhfogh left a comment

Choose a reason for hiding this comment

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

The modifications are fine, but the PR needs to be rebased on develop before it can be merged.

@jbflo jbflo changed the title [WIP] Add Specification for GPHL Json Schema form Add Specification for GPHL Json Schema form Nov 13, 2023
@jbflo jbflo marked this pull request as ready for review November 13, 2023 11:30
@marcus-oscarsson marcus-oscarsson merged commit 220b202 into develop Nov 16, 2023
@marcus-oscarsson marcus-oscarsson deleted the jb-rasmus-gphl branch January 31, 2025 08:53
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