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

ui: rename the run button #68

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

anikachurilova
Copy link
Member

@anikachurilova anikachurilova commented Feb 3, 2025

Before, it was not clear that the "Run now" button is opening a modal with configs and not immediately running the job.

Screenshot 2025-02-10 at 11 09 59

Screenshot 2025-02-10 at 11 10 23

Copy link

@tmorrell tmorrell left a comment

Choose a reason for hiding this comment

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

This is a better label

@anikachurilova anikachurilova force-pushed the harvester-reader branch 2 times, most recently from 247ef90 to 57a7df7 Compare February 7, 2025 15:17
{(props) => {
const actions_errors = props?.errors
const args = props?.values?.args;
let cleanedArgs = undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to clean up the values because of the problem in this scenario:

  1. input incorrect date into one of the fields
  2. remove everything from that input field and fill in another field
  3. the first field is not null now, but an empty string (because formik)
  4. empty string is not a valid date --> validation failed

Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure there is nothing in formik that could help with that? I think there was something related to touched values.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a touched field, but it just indicates if the field was touched or not, I would still need to implement the logic of cleaning up the value. Maybe I am missing something, but I didn't see anything useful for this in Formik.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be more proper to clean the values using formik states and methods. Doing it in the render function, you clean it up every time component is re-rendered.
Let's use the formik API: https://formik.org/docs/api/formik one of the functions, onSubmit, validate maybe? to clean only the touched and empty values and only before sending it to the backend

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -151,6 +162,11 @@ export class RunActionForm extends Component {
{!isEmpty(error) && (
<ErrorMessage
{...error}
content={
actions_errors && Object.keys(actions_errors).length > 0
Copy link
Member Author

@anikachurilova anikachurilova Feb 7, 2025

Choose a reason for hiding this comment

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

Added this in order to be able to display the exact error message of a marshmallow validation in the UI. The info is already there, just need to display it on the page now. Here is where it gets populated: https://github.com/inveniosoftware/invenio-administration/blob/main/invenio_administration/assets/semantic-ui/js/invenio_administration/src/formik/ActionForm.js#L119

Before:
Screenshot 2025-02-07 at 16 25 52

After:
Screenshot 2025-02-07 at 16 25 09

@@ -151,6 +152,11 @@ export class RunActionForm extends Component {
{!isEmpty(error) && (
<ErrorMessage
{...error}
content={
actions_errors && Object.keys(actions_errors).length > 0
? Object.values(actions_errors)[0]

Choose a reason for hiding this comment

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

Is it possible to have more than one error ? Would it make sense to display all of them?

Copy link

@jrcastro2 jrcastro2 left a comment

Choose a reason for hiding this comment

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

Peer reviewed with @ntarocco

@@ -46,7 +46,8 @@ class JobsAdminMixin:
"icon": "calendar",
},
"runs": {
"text": "Run now",
"text": "Configure and run",

Choose a reason for hiding this comment

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

Suggested change
"text": "Configure and run",
"text": "Configure run",

Copy link

@tmorrell tmorrell Feb 25, 2025

Choose a reason for hiding this comment

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

I don't think this indicates that this is where you would run the job. "Configure and run" seems clearer to me.

@kpsherva kpsherva merged commit 070d0de into inveniosoftware:master Feb 25, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released ✔️
Development

Successfully merging this pull request may close these issues.

INSPIRE to CDS-RDM harvester: implement reader component
5 participants