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

🎨 Enh: Merge secondary column's content #5740

Merged
merged 27 commits into from
Apr 29, 2024

Conversation

odeimaiz
Copy link
Member

@odeimaiz odeimaiz commented Apr 25, 2024

What do these changes do?

Having three tab pages in the secondary columns can make their content to be a bit hidden (specially the Service Options: update version, change boot mode, update resource limits). Since most of the space there was unused, this PR brings all the content to the same page.

Bonus:

  • Do not let users delete nodes while pipeline is running.

Content merged (notice the flexible labels):
SecondaryColumns

Disable main Delete Node buttons:
DeleteNodes

Related issue/s

fixes #5722
fixes ITISFoundation/osparc-issues#1398

How to test

Dev-ops checklist

@odeimaiz odeimaiz added t:enhancement Improvement or request on an existing feature a:frontend issue affecting the front-end (area group) labels Apr 25, 2024
@odeimaiz odeimaiz added this to the Enchanted Odyssey milestone Apr 25, 2024
@odeimaiz odeimaiz self-assigned this Apr 25, 2024
@odeimaiz odeimaiz changed the title WIP 🎨 Enh: Merge secondary column WIP 🎨 Enh: Merge secondary column's content Apr 26, 2024
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.0%. Comparing base (cafbf96) to head (466fdfe).
Report is 157 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5740      +/-   ##
=========================================
- Coverage    84.5%   53.0%   -31.6%     
=========================================
  Files          10     483     +473     
  Lines         214   24743   +24529     
  Branches       25     205     +180     
=========================================
+ Hits          181   13130   +12949     
- Misses         23   11561   +11538     
- Partials       10      52      +42     
Flag Coverage Δ
integrationtests 53.0% <ø> (?)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 493 files with indirect coverage changes

@odeimaiz odeimaiz marked this pull request as ready for review April 26, 2024 08:41
@odeimaiz odeimaiz requested a review from sanderegg as a code owner April 26, 2024 08:41
@odeimaiz odeimaiz changed the title WIP 🎨 Enh: Merge secondary column's content 🎨 Enh: Merge secondary column's content Apr 26, 2024
@odeimaiz odeimaiz requested a review from elisabettai April 26, 2024 08:41
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

that is very nice!!
maybe you could also align the unit with the text?
image

@odeimaiz odeimaiz enabled auto-merge (squash) April 26, 2024 09:22
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.

Wow, that's a big change, we're getting rid of the inputs and outputs tab. 🥲
I like the single tab, it helps a lot finding all the useful information in one single place.

Two small questions/suggestions:

  1. The icon of the secondary column shows gears, which is also the icon for comp. nodes and it might be confusing. I'd just delete the tab icons, since we don't have tabs anymore.
  2. Does the icon at the left of "Inputs", "Outputs", etc... mean that you can collapse it (and hide long lists of inputs/outputs if you want)? In not, I'd remove the icon.
    image

@odeimaiz
Copy link
Member Author

odeimaiz commented Apr 26, 2024

@elisabettai

  1. The icon of the secondary column shows gears, which is also the icon for comp. nodes and it might be confusing. I'd just delete the tab icons, since we don't have tabs anymore.

When you select the study entry in the tree or the selection is empty, we do show an icon on top. Also, when a file picker is selected it has its own icon. So no icon isn't an option, which icon do you propose?

  1. Does the icon at the left of "Inputs", "Outputs", etc... mean that you can collapse it (and hide long lists of inputs/outputs if you want)? In not, I'd remove the icon.

Yes, they are collapsible.

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.

fantastic!

@elisabettai
Copy link
Collaborator

When you select the study entry in the tree or the selection is empty, we do show an icon on top. Also, when a file picker is selected it has its own icon. So no icon isn't an option, which icon do you propose?

I see what you mean, but also in those cases, I don't find the icons very useful. I think they're useful if there is more than 1 tabs, so users need a way to "activate" the other tabs and recognize the different functionality. So the advantage I see in removing, is that we would clean a bit the UI from elements that we don't need.

But I also understand if you don't want to get rid of them, maybe that would require a lot of code changes for a small improvement. For the icon, I'd use the "input" (or "output") port one or a double arrow (e.g. this).

Copy link

sonarcloud bot commented Apr 26, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

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.

@odeimaiz odeimaiz merged commit aab64d4 into ITISFoundation:master Apr 29, 2024
56 checks passed
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Apr 29, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:frontend issue affecting the front-end (area group) t:enhancement Improvement or request on an existing feature
Projects
None yet
6 participants