refactor(transfer): align with select & monorepo structure by Mohammer5 · Pull Request #60 · dhis2/ui

Mohammer5 · 2020-05-07T09:17:51Z

Fixes #50

This commit does several things:

Use strings as selected values
Use strings to store which options are highlighted
Removes jest test abstractions
Simplifies internal names
Replaces common.js files with files whose names match their content
Moves the Transfer component from core to widgets
Adds the .js extension to all import statements where applicable
Provides the correct disabled & dataTest props to the re-order up
button's icon

BREAKING CHANGE: The Transfer component now expects options to be passed in as objects, not as children. Custom components can be provided via the optionComponent prop for all options or via the component property on an individual option.

BREAKING CHANGE: The Transfer component now expects strings as selected
values instead of option objects.

BREAKING CHANGE: The Transfer component is now part of widgets

Mohammer5 · 2020-05-07T09:19:33Z

👍

cypress · 2020-05-07T09:30:24Z

Test summary

438 • 0 • 0 • 0

Run details


Project	ui
Status	Passed
Commit	`53a2e81`
Started	May 28, 2020 6:36 AM
Ended	May 28, 2020 6:44 AM
Duration	08:10 💡
OS	Linux Ubuntu Linux - 18.04
Browser	Electron 80

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Mohammer5 · 2020-05-07T09:33:48Z

@HendrikThePendric The multi select feature failed, it does not fail locally. I assume that's another issue with the requestAnimationFrame timing, right?

ghost · 2020-05-11T08:35:50Z

@HendrikThePendric The multi select feature failed, it does not fail locally. I assume that's another issue with the requestAnimationFrame timing, right?

Yeah I think we need to take another look at the select in general. ~~I think we can find another way to deal with this than using RAF, and~~ with the recent refactors we've had of the select I suspect there are bits of it that need a little love.

I've added an issue for this here: #64.

HendrikThePendric · 2020-05-11T09:05:01Z

The multi select feature failed, it does not fail locally. I assume that's another issue with the requestAnimationFrame timing, right?

Oh man 🤦 that's very unfortunate. It's not really an issue with requestAnimationFrame, but rather with the ResizeObserver. I had already implemented a helper called waitForResizeObserver to fix it, but apparently that isn't super robust either. I really don't know of another way to address this problem.

ghost · 2020-05-11T09:11:25Z

The multi select feature failed, it does not fail locally. I assume that's another issue with the requestAnimationFrame timing, right?

Oh man 🤦 that's very unfortunate. It's not really an issue with requestAnimationFrame, but rather with the ResizeObserver. I had already implemented a helper called waitForResizeObserver to fix it, but apparently that isn't super robust either. I really don't know of another way to address this problem.

It's weird, I tried rerunning the integration tests, but now they're failing completely:

HendrikThePendric · 2020-05-20T10:02:29Z

I've had a quick attempt at reviewing these PRs but I got the distinct impression that the vast majority was just a matter of moving things around. @Mohammer5 would it be possible to indicate where you have actually changed an existing implementation?

Mohammer5 · 2020-05-20T13:51:42Z

yeah, so most work has bee done on functions that use the value of the selected prop or a derived value of that prop. It's quite a few files as that was the biggest change because two things have changed at the same time:

Use strings instead of objects
Use options instead of children prop

If you want to, we could schedule a meeting where I go over what's changed @HendrikThePendric @ismay

HendrikThePendric

This is looking very good. Only thing I am a little worried about is the way we work with custom options. I've left a suggestion for changing that.

HendrikThePendric · 2020-05-25T14:46:44Z

@@ -75,9 +64,9 @@ export const multiSelectedPropType = propTypes.arrayOf(singleSelectedPropType)
 * rest of the library
 */
 export const Transfer = ({


I've been thinking about rendering custom options and how we could stay a little bit more consistent here with other components that may require customisation. It's quite difficult to come up with something that is exactly the same, since this options prop pattern is fundamentally different than the children pattern we use elsewhere. After chatting a little bit with @ismay I came up with an idea (not sure whether Ismay will fully agree).

I'll place a few comments in the component code to list the changes I'd like to suggest.

HendrikThePendric · 2020-05-25T14:47:34Z

    leftFooter,
    leftHeader,
    maxSelections,
+    optionComponent: TransferOption,


change to an optional prop called renderOption of type function

HendrikThePendric · 2020-05-25T14:54:24Z

+                        const Option = option.component || TransferOption
+                        const highlighted = !!highlightedSourceOptions.find(
+                            highlightedSourceOption =>
+                                highlightedSourceOption === option.value
+                        )
+
+                        return (
+                            <Option
+                                key={option.value}
+                                disabled={option.disabled}
+                                option={option}
+                                highlighted={highlighted}
+                                {...getOptionClickHandlers(
+                                    option,
+                                    selectSingleOption,
+                                    toggleHighlightedSourceOption
+                                )}
+                            />
+                        )


Change this part to this

const renderFn = option.render || renderOption const optionProps = { disabled: option.disabled, option: option, highlighted: !!highlightedSourceOptions.find( highlightedSourceOption => highlightedSourceOption === option.value ), ...getOptionClickHandlers( option, selectSingleOption, toggleHighlightedSourceOption ), } return typeof renderFn === 'function' ? ( renderFn(optionProps) ) : ( <TransferOption {...optionProps} /> )

HendrikThePendric · 2020-05-25T14:55:08Z

+        propTypes.shape({
+            label: propTypes.string.isRequired,
+            value: propTypes.string.isRequired,
+            component: propTypes.func,


change to render

HendrikThePendric · 2020-05-25T14:55:22Z

    leftFooter: propTypes.node,
    leftHeader: propTypes.node,
    maxSelections: propTypes.oneOf([1, Infinity]),
+    optionComponent: propTypes.func,


change to renderOption

ghost · 2020-05-25T15:27:35Z

@HendrikThePendric and I talked a bit about this. In general we're thinking of the same thing, though my preference would be a slightly more conservative approach. In pseudo code I'm thinking of:

// Allow the user to override the default option rendering
renderOption = option.renderOption || defaultOptionRenderer

// ...

// option rendering just maps over all the options,
// and calls renderOption with the args needed for rendering options
// renderOption is expected to return (react) renderable content
options.map(option => {
  renderOption({ option, etc. })
})

So what I'm omitting compared to @HendrikThePendric's approach is allowing the render prop on an option in the options array. the way I see it, you can already implement everything you want in the renderOption prop passed to the <Transfer />, so I'd omit allowing render props per option for the sake of simplicity. We could always add it later on if it turns out to be necessary.

HendrikThePendric · 2020-05-25T15:31:38Z

We also briefly discussed the option of adhering to the children API for v5, but not sure that is feasible at all.... Perhaps we can discuss things again tomorrow?

ghost · 2020-05-25T15:33:11Z

We also briefly discussed the option of adhering to the children API for v5, but not sure that is feasible at all.... Perhaps we can discuss things again tomorrow?

Yeah that'd be nice 👍. If we can align on everything tomorrow then we could maybe finish up the remaining tasks for v5 this week.

Mohammer5 · 2020-05-26T08:32:05Z

I've introduced this the way @ismay proposed because there's a performance implication. The prop should either always expect a component or a function.

I've removed the custom option functionality from the individual option items as that can be done through the renderOption function. We can easily add it back if ever needed.

HendrikThePendric

👍 LGTM

This commit does several things: * Use strings as selected values * Use strings to store which options are highlighted * Removes jest test abstractions * Simplifies internal names * Replaces `common.js` files with files whose names match their content * Moves the Transfer component from `core` to `widgets` * Adds the `.js` extension to all import statements where applicable * Provides the correct `disabled` & `dataTest` props to the re-order up button's icon BREAKING CHANGE: The Transfer component now expects strings as selected values instead of option objects. BREAKING CHANGE: The Transfer component is now part of `widgets`

dhis2-bot · 2020-05-28T07:15:53Z

🎉 This PR is included in version 5.0.0-alpha.20 🎉

The release is available on:

Your semantic-release bot 📦🚀

dhis2-bot · 2020-05-28T14:22:10Z

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Mohammer5 requested a review from a team as a code owner May 7, 2020 09:17

Mohammer5 mentioned this pull request May 7, 2020

ui@5 #56

Merged

15 tasks

HendrikThePendric mentioned this pull request May 11, 2020

Revisit Select #64

Closed

ghost mentioned this pull request May 11, 2020

feat(widgets): add default translated texts #61

Merged

14 tasks

ghost added this to the v5.0.0 milestone May 14, 2020

HendrikThePendric reviewed May 25, 2020

View reviewed changes

Mohammer5 force-pushed the Transfer branch from 5c31ff1 to 4d6f0a9 Compare May 26, 2020 08:30

HendrikThePendric approved these changes May 26, 2020

View reviewed changes

Mohammer5 added 2 commits May 27, 2020 11:52

refactor: use "renderOption" instead of "optionComponent"

06b32df

Mohammer5 force-pushed the Transfer branch from 80a71f2 to 6b3eaf3 Compare May 27, 2020 09:53

refactor: use option's value only as payload of click handlers

53a2e81

Mohammer5 force-pushed the Transfer branch from 6b3eaf3 to 53a2e81 Compare May 28, 2020 06:34

Mohammer5 merged commit c15477d into alpha May 28, 2020

Mohammer5 deleted the Transfer branch May 28, 2020 07:10

dhis2-bot added the released on @alpha label May 28, 2020

dhis2-bot added the released label May 28, 2020

Conversation

Mohammer5 commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mohammer5 commented May 7, 2020

Uh oh!

cypress bot commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Uh oh!

Mohammer5 commented May 7, 2020

Uh oh!

ghost commented May 11, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HendrikThePendric commented May 11, 2020

Uh oh!

ghost commented May 11, 2020

Uh oh!

HendrikThePendric commented May 20, 2020

Uh oh!

Mohammer5 commented May 20, 2020

Uh oh!

HendrikThePendric left a comment

Choose a reason for hiding this comment

Uh oh!

HendrikThePendric May 25, 2020

Choose a reason for hiding this comment

Uh oh!

HendrikThePendric May 25, 2020

Choose a reason for hiding this comment

Uh oh!

HendrikThePendric May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HendrikThePendric May 25, 2020

Choose a reason for hiding this comment

Uh oh!

HendrikThePendric May 25, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented May 25, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HendrikThePendric commented May 25, 2020

Uh oh!

ghost commented May 25, 2020

Uh oh!

Mohammer5 commented May 26, 2020

Uh oh!

HendrikThePendric left a comment

Choose a reason for hiding this comment

Uh oh!

dhis2-bot commented May 28, 2020

Uh oh!

dhis2-bot commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mohammer5 commented May 7, 2020 •

edited

Loading

cypress bot commented May 7, 2020 •

edited

Loading

ghost commented May 11, 2020 •

edited by ghost

Loading

HendrikThePendric May 25, 2020 •

edited

Loading

ghost commented May 25, 2020 •

edited by ghost

Loading