feat(layout): add foundation

[Birkbjo]

No description provided.

[Mohammer5]

Centralizing the open state and make nav children stateless

With the simplication of the forceOpen and previous state in the SidenavParent (see this comment), you could centralize the state for which parent is open in the Sidebar component by using useState that has an array with all opened labels. That way you can remove the entire HidePreventUnmount logic / component, which seems unnecessarily complicated to me. The approach of centralizing the state in a single component and than rendering only
stateless components seems to be simpler to me.

components/utils folder

src/components/utils should have a better name. I'd argue for splitting it up into the components that are inside. src/components is already an "abstraction" (components that are shared), adding another architectural layer of abstraction makes the architecture more complex & complicated. It's also debatable which component is a util and which is not, which introduces ambiguity. With my suggestion to centralize/simplify the state in the sidebar (see one headline above) and subsequently being able to remove HidePreventUnmount, we can get rid of this folder as there's nothing in there right now

Nav filtering

filtering doesn't include group labels (e.g. "Data elements" as search would result in an empty list. I suppose you could argue that "Data element" should be enough, but it wouldn't include the overview pages)

Components for overview views

With all overview pages being located in src/pages/overview, we can move the components inside src/components/overview into src/pages/overview and co-locate the related code which won't be used anywhere else. Same applies to what can be found in src/components/card

Errors when opening app in the browser

This is happening because some file names are lower case but the import statements use a capitalized folder name and vice versa. It's weird that these errors don't show up on the cli.

Compiled with problems:

ERROR in ./src/D2App/App.js 5:104-120

export 'ConfiguredRouter' (imported as 'ConfiguredRouter') was not found in './app/' (possible exports: AppWrapper, Layout)


ERROR in ./src/D2App/app/index.js 3:0-34

export 'Layout' (reexported as 'Layout') was not found in './layout' (module has no exports)


ERROR in ./src/D2App/app/index.js 5:0-44

Module not found: Error: Can't resolve './Router' in '/shared/development/dhis2/maintenance-app-beta/.d2/shell/src/D2App/app'


ERROR in ./src/D2App/app/layout/index.js 3:0-34

Module not found: Error: Can't resolve './Layout' in '/shared/development/dhis2/maintenance-app-beta/.d2/shell/src/D2App/app/layout'

[Mohammer5]

Suggested change

	export { ConfiguredRouter } from "./Router";
	export { ConfiguredRouter } from "./router";

[Birkbjo]

Yeah sorry about these, last thing I worked on was fixing up the file names, and I realized I never finished and didn't push my latest work.

[Mohammer5]

Suggested change

	export { Layout } from './Layout';
	export { Layout } from './layout';

[Mohammer5]

Suggested change

	const DataElements = lazy(() => import("./DataElements"));
	const DataElements = lazy(() => import("./dataElements"));

[Mohammer5]

Mozafar and I agreed that we want to merge PR 323 asap. So I extracted open comments into separate sub tasks here, so they can be worked on right away after merging the PR.

Immediate follow-up todos: https://dhis2.atlassian.net/browse/DHIS2-15071

[Birkbjo]

src/components/utils should have a better name. I'd argue for splitting it up into the components that are inside. src/components is already an "abstraction" (components that are shared), adding another architectural layer of abstraction makes the architecture more complex & complicated. It's also debatable which component is a util and which is not, which introduces ambiguity. With my suggestion to centralize/simplify the state in the sidebar (see one headline above) and subsequently being able to remove HidePreventUnmount, we can get rid of this folder as there's nothing in there right now

I'm not sure that utils and src/components would be the same abstraction. It's for utility components. I think it's perfectly valid to have a folder like that, and we will most likely end up with components that fit that, instead of creating separate folders for small utility components like these.

We can probably do without HidePreventUnmount, but I do think it solves a valid problem and actually simplifies the state handling in this case. But if we don't want to keep the previous state after clearing the filter (as @cooper-joe seems to suggest), it's not needed for now.

Nav Filtering

filtering doesn't include group labels (e.g. "Data elements" as search would result in an empty list. I suppose you could argue that "Data element" should be enough, but it wouldn't include the overview pages)

I have added logic for this now. If there's a match in the parent label, all children routes are shown. Do you agree with this logic, @cooper-joe ?

[Mohammer5]

I'm not sure that utils and src/components would be the same abstraction. It's for utility components.

That's not what I was trying to say, but that having two nested abstraction-folders is increasing complexity.

I think it's perfectly valid to have a folder like that, and we will most likely end up with components that fit that, instead of creating separate folders for small utility components like these.

Another point I made is that it can be debatable which component is a util and which is not, which introduces ambiguity. I think HidePreventUnmount is a very good example for what I'm trying to point out. As a developer, I know that src/components will contain shared components, the abstraction level is quite clear. Then inside that folder, there's the utils folder which I wouldn't know what to find inside conceptually. Why would HidePreventUnmount be a utility? What's the difference between that component and Loader for example, which could be considered a utility component but is not in the utils folder. Just like DefaultLazyLoad.

Considering my comment to move what's inside components/card & components/overview into pages/overview (as that is the only place where these will be used, they're not really that "shared"), everything that'd be left in components could be considered a utility component.

So having utils seems very ambiguous to me and therefore also reduce discoverability due to the extra level of nesting. I can't see the advantage or necessity of that folder yet, but on the other hand I can see disadvantages and lack of clarity.

We can probably do without HidePreventUnmount, but I do think it solves a valid problem and actually simplifies the state handling in this case. But if we don't want to keep the previous state after clearing the filter (as @cooper-joe seems to suggest), it's not needed for now.

I can't see the component being used anywhere right now.

[cooper-joe]

I have added logic for this now. If there's a match in the parent label, all children routes are shown. Do you agree with this logic, @cooper-joe ?

Yeah, I think we can go for this. The alternative of hiding children would be too confusing (as parent labels are not navigation).

[Birkbjo]

Another point I made is that it can be debatable which component is a util and which is not, which introduces ambiguity.

Why would HidePreventUnmount be a utility? What's the difference between that component and Loader for example, which could be considered a utility component but is not in the utils folder. Just like DefaultLazyLoad.

Yeah I agree that it can be a bit arbitrary. I'm not sure if it's ambiguous though - it's just a decision where it lives. And everything we do are decisions right? And can look arbitrary if you don't know the context or reason behind a decision. But that doesn't mean it's ambiguous. utils is just a catch-all for small utility components that doesn't "deserve" it's own folder. I do think both Loader and DefaultLazyLoad could be in a utils-folder, but as they have styles attached, and thus not in one file I decided to move it to their own folders. I think introducing a folder for just one file is a bit overkill, and I wouldn't even be sure what to call the concept of HidePreventUnmount - it's just a small utility component.

[Mohammer5]

@Birkbjo I've created a separate issue (#324 / DHIS2-15080) so we can talk about it. I'll merge this PR to unblock work.

[dhis2-bot]

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

• AppHub release
• GitHub release

Your semantic-release bot 📦🚀