Skip to content

✨ Frontend: Expose inputs required property #5899

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 34 commits into from
Jun 3, 2024
Merged

✨ Frontend: Expose inputs required property #5899

merged 34 commits into from
Jun 3, 2024

Conversation

odeimaiz
Copy link
Member

@odeimaiz odeimaiz commented May 30, 2024 •
edited
Loading

What do these changes do?

This PR exposes the node's inputsRequired attribute to the frontend. Users will now be able to make inputs required.

RequiredInputs

Related issue/s

related to #5838
related to #5845

How to test

Dev-ops checklist

@odeimaiz odeimaiz self-assigned this May 30, 2024
@odeimaiz odeimaiz added this to the Leeroy Jenkins milestone May 30, 2024
@odeimaiz odeimaiz added the a:frontend issue affecting the front-end (area group) label May 30, 2024
@odeimaiz odeimaiz marked this pull request as ready for review May 31, 2024 12:34
@odeimaiz odeimaiz requested review from jsaq007 and ignapas as code owners May 31, 2024 12:34
@odeimaiz odeimaiz changed the title ✨ Frontend: Make inputs required ✨ Frontend: Expose inputs required property May 31, 2024
matusdrobuliak66
matusdrobuliak66 approved these changes May 31, 2024
View reviewed changes
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

💯

elisabettai
elisabettai approved these changes May 31, 2024
View reviewed changes
Copy link
Collaborator

@elisabettai elisabettai left a comment

Choose a reason for hiding this comment

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

Nice!

Maybe you can add a tooltip saying what happens when you set required.

@odeimaiz
Copy link
Member Author

odeimaiz commented May 31, 2024 •
edited
Loading

Maybe you can add a tooltip saying what happens when you set required.

The tooltip will say: "Required input: Without it, the service will not start/run."

@elisabettai Is that fine?

pcrespov
pcrespov approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx

GitHK
GitHK reviewed May 31, 2024
View reviewed changes
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Nice! It would be wonderful if when hovering ofter the label, in the description it would also say that that input is required. Also when hovering over the asterisk.

@elisabettai
Copy link
Collaborator

elisabettai commented May 31, 2024 •
edited
Loading

Maybe you can add a tooltip saying what happens when you set required.

The tooltip will say: "Required input: Without it, the service will not start/run."

@elisabettai Is that fine?

It sounds good. I also just noticed that you change the string when the option is selected, and it is a bit confusing (like it was for Service Autostart).
I know you don't like a switch button, and without adding a color to it, it might be more confusing.
What about:

  • "Mandatory input: 🟩 " (it is not required)
  • "Mandatory input: ✅ " (it is required)

If you go in that direction, than the tooltip should say: "Mandatory input: without it, the connected service will not start/run"

@odeimaiz odeimaiz enabled auto-merge (squash) May 31, 2024 14:13
@odeimaiz odeimaiz disabled auto-merge May 31, 2024 16:03
@odeimaiz odeimaiz enabled auto-merge (squash) June 3, 2024 08:13
jsaq007
jsaq007 approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@jsaq007 jsaq007 left a comment

Choose a reason for hiding this comment

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

Looks good

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@odeimaiz odeimaiz merged commit dd7da65 into ITISFoundation:master Jun 3, 2024
51 checks passed
This was referenced Jun 10, 2024
Closed
Closed
@odeimaiz
Copy link
Member Author

For the Release Notes:

RequiredInputs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GitHK GitHK GitHK left review comments

@jsaq007 jsaq007 jsaq007 approved these changes

@elisabettai elisabettai elisabettai approved these changes

@pcrespov pcrespov pcrespov approved these changes

@sanderegg sanderegg sanderegg approved these changes

@matusdrobuliak66 matusdrobuliak66 matusdrobuliak66 approved these changes

@ignapas ignapas Awaiting requested review from ignapas

Assignees

@odeimaiz odeimaiz

Labels
a:frontend issue affecting the front-end (area group)
Projects
None yet
Milestone
Leeroy Jenkins
Development

Successfully merging this pull request may close these issues.
7 participants
@odeimaiz @elisabettai @jsaq007 @GitHK @pcrespov @sanderegg @matusdrobuliak66