Skip to content

feat(web): adapt storage content according to the scenario#1972

Merged
ancorgs merged 21 commits intoagama-project:storage-config-uifrom
joseivanlopez:storage-layout-cases
Feb 14, 2025
Merged

feat(web): adapt storage content according to the scenario#1972
ancorgs merged 21 commits intoagama-project:storage-config-uifrom
joseivanlopez:storage-layout-cases

Conversation

@joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Feb 4, 2025

Adapt the content of the storage page according to the situation:

  • No available devices: show empty state with options to configure devices (iSCSI, DASD, etc).
  • Editable config and successful result: show Installation Devices and Result sections.
  • Editable config and no result: show error and Installation devices section.
  • No editable config and successful result: show alert with reset option and Result section.
  • No editable config and no result: show empty state with config errors (if any) and reset option.

The encryption section is dropped for now.

Note: for now, the config is considered as non-editable if there are config errors. In the future, some components like ConfigEditor could be improved to make possible fixing an invalid config.

Screenshots

Normal screen

ly-normal

Correct settings but impossible setup

ly-failed

Unhandled settings but possible setup

lt-unsupported

Unhandled settings and impossible setup

ly-wrong

Invalid settings

ly-invalid

No target disks in s390

ly-activate-all

No target disks in other architectures

ly-iscsi

@joseivanlopez joseivanlopez force-pushed the storage-layout-cases branch 6 times, most recently from deb6c6b to 2237aa3 Compare February 7, 2025 14:28
@joseivanlopez joseivanlopez changed the title feat(storage): show proper layout accoding to the scenario feat(storage): show content accoding to the scenario Feb 7, 2025
@joseivanlopez joseivanlopez changed the title feat(storage): show content accoding to the scenario feat(storage): adapt content accoding to the scenario Feb 7, 2025
@joseivanlopez joseivanlopez force-pushed the storage-layout-cases branch 8 times, most recently from 9f22956 to bb677de Compare February 11, 2025 12:02
@joseivanlopez joseivanlopez changed the title feat(storage): adapt content accoding to the scenario feat(storage): adapt content according to the scenario Feb 11, 2025
@joseivanlopez joseivanlopez changed the title feat(storage): adapt content according to the scenario feat(web): adapt storage content according to the scenario Feb 11, 2025
@joseivanlopez joseivanlopez marked this pull request as ready for review February 11, 2025 14:47
errors: Issue[];
};

function ErrorsEmptyState({ errors }: ErrorsEmptyStateProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have gone too far accumulating functionality on this file (just look at the size of the "import" section). Can't we extract the empty pattern component(s) to separate files? Similar to what is done for ProposalTransactionalInfo and ProposalFailedInfo?

Copy link
Contributor Author

@joseivanlopez joseivanlopez Feb 13, 2025

Choose a reason for hiding this comment

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

I don't know if extracting single-usage-components like these, which don't have any kind of logic, is good or not.

IMHO, in cases like this, I prefer to have such components defined directly there in the only file where they are needed. Otherwise, there is a lot of chance of forgetting about them when the main component is refactored (that has already happened). Of course, there could be exceptions, like having complex components with too much logic.

But I can extract them if you guys prefer to go with that approach.

Copy link
Contributor Author

@joseivanlopez joseivanlopez Feb 14, 2025

Choose a reason for hiding this comment

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

We decided to keep that components directly there, but in the future we will extract them to separate files, testing each component separately. Moreover, each component will have the logic to decide whether to render any content. The proposal page will simply mount all those components.

Copy link
Contributor

@dgdavid dgdavid Feb 14, 2025

Choose a reason for hiding this comment

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

Why did you guys decide to postpone something agreed less than 3 hours ago? Just curious because it is, again, a kick to the front-end code.

I start having the feeling that does not payoff to invest time in meetings.

Copy link
Contributor

Choose a reason for hiding this comment

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

In a few words: "time pressure" 😓

Copy link
Contributor Author

@joseivanlopez joseivanlopez Feb 17, 2025

Choose a reason for hiding this comment

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

Why did you guys decide to postpone something agreed less than 3 hours ago? Just curious because it is, again, a kick to the front-end code.

JFTR: because just after talking to you, the PO decided to go with the current code if it works. As @imobachgs said, time pressure. We agreed a) to add a FIXME, b) follow the agreed approach from now on, c) redo this code (and many other things) once time permits.

I start having the feeling that does not payoff to invest time in meetings.

I disagree. Our agreement is ok and we will follow it from now on.

@coveralls
Copy link

coveralls commented Feb 13, 2025

Pull Request Test Coverage Report for Build 13334350448

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-47.4%) to 25.046%

Totals Coverage Status
Change from base Build 13305098023: -47.4%
Covered Lines: 1752
Relevant Lines: 6995

💛 - Coveralls

@joseivanlopez joseivanlopez force-pushed the storage-layout-cases branch 2 times, most recently from 221c24e to d13b85f Compare February 14, 2025 14:39
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

This LGTM.

I'm not merging because the last commit was added by me (together with all the screenshots at the description). Somebody would need to review those texts. Once the texts are reviewed, we can merge.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Just some suggestions.

<Content component="p">{_("Do you want to reset to the default settings?")}</Content>
<Content component="p">
{_(
"You may want to discard those settings and start from scratch with a simple configuration.",
Copy link
Contributor

Choose a reason for hiding this comment

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

those or these?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say "those" because the settings are not really displayed. And the reference to them is at a previous paragraph. Both sound like "distance" to me.

@ancorgs ancorgs merged commit 5239131 into agama-project:storage-config-ui Feb 14, 2025
5 checks passed
@imobachgs imobachgs mentioned this pull request Feb 26, 2025
imobachgs added a commit that referenced this pull request Feb 26, 2025
dgdavid added a commit that referenced this pull request Jun 2, 2025
The `toValidationError` function is no longer in use. It was originally
introduced in the storage area implementation (#540, commit 1fc1f6f)
but its usage was gradually phased out across later changes (e.g., #1112),
until it became fully unused in commit 9cfc9c7 (part of #1972).

Removing it to reduce dead code and simplify the utils namespace.
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.

5 participants