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

[web] Restructure src by creating a module per installer area #325

Merged
merged 15 commits into from
Nov 23, 2022

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Nov 21, 2022

Problem

As D-Installer evolves, the src directory becomes messy and out of control.

Solution

To create a module per installation area by using re-exports and to use aliases for easing the way components are imported instead of using complex relative paths.

Other interesting links

@coveralls
Copy link

coveralls commented Nov 21, 2022

Coverage Status

Coverage increased (+0.2%) to 74.757% when pulling 8dbf94d on restructure-src into 2de4014 on master.

@dgdavid dgdavid changed the title Restructure src [web] Restructure src by creating a module per installer area Nov 22, 2022
web/src/components/core/index.js Show resolved Hide resolved
* find current contact information at www.suse.com.
*/

export { default as LanguageSelector } from "./LanguageSelector";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe LanguageSelector can be named just Selector now that it is part of the language module. On the other hand, then it might or might not be imported as LanguageSelector or so in some places.

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 do all those changes about renaming things as part of a separate PR.


import DBusError from "./DBusError";
import { DBusError } from "@components/layout";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it is time for renaming DBusError component. What do you think?

export { default as Layout } from "./Layout";
export { default as Center } from "./Center";
export { default as DBusError } from "./DBusError";
export { default as LoadingEnvironment } from "./LoadingEnvironment";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is PR is a good opportunity for making LoadingEnvironment a more generic Loading component. In fact, it already receives a text prop that can be accordingly adapted too. What do you think?

Comment on lines +29 to +30
export { default as NetworkWifiStatus } from "./NetworkWifiStatus";
export { default as NetworkWiredStatus } from "./NetworkWiredStatus";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can get rid of Network prefixes now. Do you agree?

* find current contact information at www.suse.com.
*/

export { default as Network } from "./Network";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about renaming this compononent to NetworkOverview. Tha way, we can go for a NetworkOverview or NetworkSummary for the summary and NetworkPage for the section page. Simlarly, we could have StorageOverview and StoragePage, LanguageOverview and LanguagePage and so on. What is more, they could be simply an Overview and Page components inside their modules...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I vote for it: Network/Overview, Network/Page, etc. But maybe in a separate PR.

Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

web/src/components/layout/index.js Show resolved Hide resolved
* find current contact information at www.suse.com.
*/

export { default as Network } from "./Network";
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I vote for it: Network/Overview, Network/Page, etc. But maybe in a separate PR.

web/src/components/core/index.js Show resolved Hide resolved
* find current contact information at www.suse.com.
*/

export { default as LanguageSelector } from "./LanguageSelector";
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 do all those changes about renaming things as part of a separate PR.

@dgdavid dgdavid marked this pull request as ready for review November 22, 2022 22:34
@dgdavid dgdavid merged commit 49e6a2a into master Nov 23, 2022
@dgdavid dgdavid deleted the restructure-src branch November 23, 2022 10:13
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.

None yet

3 participants