Post-Transfer merge improvements about ui HOT 13 CLOSED

I'm not fond of the common pieces. I had a hard time keeping track if I was looking at Transfer/e2e/common.js, tests/common/*, or Transfer/common.js.

I really don't like the amount of abstraction we have in our tests in general. I think my preferred solution to most of the abstractions we have in our tests would be to not abstract at all. This article expresses it well: https://mtlynch.io/good-developers-bad-tests

The problem is that abstracting our tests too much makes them harder to understand:

Test code is a different beast. A good unit test is often small enough that a developer can conceptualize all the logic at once. Adding layers of abstraction to test code increases its complexity. Tests are a diagnostic tool, so they should be as simple and obvious as possible.

Good test code is no different. It should produce clear results without forcing the reader to jump through multiple levels of indirection. Developers often lose sight of this because it differs from how they learned to write production code.

To write excellent tests, a developer must align their engineering decisions with the goals of test code. Most importantly, tests should maximize simplicity while minimizing abstraction. A good test allows the reader to understand intended behavior and diagnose issues without ever leaving the test function.

Abstractions decrease LOC and repetition but increase complexity. In dealing with unit tests I prefer more verbose, simpler tests. So instead of taking the abstractions further, I'd say that the abstractions themselves are the problem, and that we should cut down on them in our unit tests as much as possible.

Please specify the extension when importing (this is the code style in UI !). Using import { foo } from 'directory' or import { foo } from 'file' to import from directory/index.js or file.js is not ES compatible -- it is a Node-ism that is supported by Webpack. Pure ES tools do choke on that unless it is Babelified or handled later. Would be nice to have a ESLint for this, as it makes it a lot easier to understand exactly where the code is imported from.

Yes this can be configured with the import plugin. A good migration path could be to enable the rule locally in some repos first (opt-in). And then at a certain point enabling it globally as well, where people could disable it locally (opt-out).

We're still testing the import plugin in an alpha of cli-style. If it makes it out we could do the above.

from ui.

Please specify the extension when importing (this is the code style in UI !). Using import { foo } from 'directory' or import { foo } from 'file' to import from directory/index.js or file.js is not ES compatible -- it is a Node-ism that is supported by Webpack. Pure ES tools do choke on that unless it is Babelified or handled later. Would be nice to have a ESLint for this, as it makes it a lot easier to understand exactly where the code is imported from.

We should have an eslint rule for this. My auto-importing vim plugin does not have an option for this (especially as we don't want that in every repo).

from ui.

Transfer uses InputField, so technically it cannot be a core component. The choice was between moving Transfer to widgets or refactoring to use Field+Input. I did the latter in this PR, but maybe we want it to be a widget (with translations, e.g.)

Agree, I think it should be moved to widgets (conceptually it also makes sense to be side by side with the Org Unit Tree; They're dhis2 data/logic specific)

from ui.

Agree, I think it should be moved to widgets (conceptually it also makes sense to be side by side with the Org Unit Tree; They're dhis2 data/logic specific)

OK, let's do that in the refactor:

1. Move away from { value, label } objects for selected values and instead using string values.
2. Fix the Jest config.
3. Move Transfer from core to widgets.

from ui.

I did a "best effort" merge last night since you mentioned that the Transfer will need some refactoring once it's moved to UI. This should be part of that.
Source comment

1. We are moving away from { value, label } objects for selected values and instead using string values.

This and the jest config is what I was talking about

from ui.

I'm not fond of the common pieces. I had a hard time keeping track if I was looking at Transfer/e2e/common.js, tests/common/*, or Transfer/common.js.

How would you extract these instead?

from ui.

The __tests__/common has several options:

• Add a private package to packages/test-utils, then we can import createChildren from ui-test-utils (I like this one).
• Create a folder in packages/core/src named utils and put createChildren.js there.
• Don't automate this.

I really like the first option. We should keep an eye on not putting too much in there though.

from ui.

I'm not fond of the common pieces. I had a hard time keeping track if I was looking at Transfer/e2e/common.js, tests/common/*, or Transfer/common.js.

How would you extract these instead?

I'd choose a different name than common.js. Not sure what though. 😅

from ui.

Another improvement I found:
It seems like I accidentally messed up a value passed to disabled prop of the actual icon in packages/core/src/Transfer/ReorderingActions.js (Line 38)

from ui.

We should have an eslint rule for this. My auto-importing vim plugin does not have an option for this (especially as we don't want that in every repo).

Looks like eslint-plugin-import which @ismay recently added to cli-style (alpha currently) has a rule for this

It should be noted that most of our codebases use bare file imports, so if we enforce it broadly it will cause a lot of changes. We could turn it on for this repo only easily enough though!

from ui.

if we enforce it broadly it will cause a lot of changes

Afaik we only want this in our ui libraries, not sure though (CC: @varl )

from ui.

🎉 This issue has been resolved in version 5.0.0-alpha.20 🎉

The release is available on:

• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

from ui.

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

from ui.