Skip to content

feat(transfer): add requested improvements #220

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.

Sign up for GitHub

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

Merged
Mohammer5 merged 10 commits into from
Aug 14, 2020
Merged

feat(transfer): add requested improvements #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2a51e79
feat(transfer): add onSourceEndReached & onPickedEndReached callbacks
Mohammer5 Jul 28, 2020
479c091
feat(transfer): allow filtering the picked options
Mohammer5 Jul 28, 2020
ce6fa84
feat(transfer): add loading props
Mohammer5 Jul 28, 2020
8067f65
chore: fix broken cypress tests after refactoring
Mohammer5 Jul 29, 2020
4eb58c7
refactor(transfer): add data test to intersection detector
Mohammer5 Jul 29, 2020
7996144
test(transfer): cover end of list notification with cypress tests
Mohammer5 Jul 29, 2020
aad40f0
test(transfer): cover loading lists with cypress tests
Mohammer5 Jul 29, 2020
2da5fcd
refactor: extract logic for contstructing filtering variables
Mohammer5 Aug 11, 2020
74ff168
chore: fix broken cypress tests
Mohammer5 Aug 11, 2020
13a1804
refactor: make prop names consistent
Mohammer5 Aug 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions packages/widgets/src/Transfer/EndIntersectionDetector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { IntersectionDetector } from '@dhis2/ui-core'
import propTypes from '@dhis2/prop-types'
import React from 'react'

export const EndIntersectionDetector = ({
rootRef,
onEndReached,
dataTest,
}) => (
<div data-test={dataTest}>
<IntersectionDetector
rootRef={rootRef}
onChange={({ isIntersecting }) => isIntersecting && onEndReached()}
/>

<style jsx>{`
div {
width: 100%;
height: 50px;
position: absolute;
z-index: -1;
bottom: 0;
left: 0;
}
`}</style>
</div>
)

EndIntersectionDetector.propTypes = {
rootRef: propTypes.shape({
current: propTypes.instanceOf(HTMLElement),
}).isRequired,
onEndReached: propTypes.func.isRequired,
dataTest: propTypes.string,
}
144 changes: 144 additions & 0 deletions packages/widgets/src/Transfer/OptionsContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
import { CircularLoader } from '@dhis2/ui-core'
import { spacers } from '@dhis2/ui-constants'
import React, { Fragment, useEffect, useRef, useState } from 'react'
import propTypes from '@dhis2/prop-types'

import { EndIntersectionDetector } from './EndIntersectionDetector.js'

export const OptionsContainer = ({
Copy link
Copy Markdown

@ghost ghost Jul 30, 2020

Choose a reason for hiding this comment

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

I know we talked about the IntersectionDetector not working properly in some cases. I suspect that you've addressed that issue here with the resize observer? Or is it still an issue?

I think what I'd prefer as a reviewer to fix the intersection detector bug is a separate PR with a failing test as the initial commit. So that we can then test our solutions against that. From the reviewer's (or my) perspective that'd be a nice approach.

One other thing I'm still skeptical about with the IntersectionDetector component by the way is that the name is directly tied to the api. The way I see it, we're interested in detecting visibility. That's why I preferred VisibilityDetector in the other PR for this component, since it seemed like that was what it had to do.

Now instead of the function we've tied the naming to the api. And now that we're encountering issues where the api isn't fulfilling our needs of detecting visibility we can't fix it within the component, because it doesn't fit the scope of it. I think I'd still prefer a visibility detector component that neatly packages that functionality. That way we can just use that component to detect when the end of the list has been reached, and fix bugs within that component.

Copy link
Copy Markdown
Contributor Author

@Mohammer5 Mohammer5 Aug 11, 2020

Choose a reason for hiding this comment

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

Yes, I've addressed it already. The problem with the visibility component in this case is, that the fix had to be done for a DOM element that's specific to the transfer.

I'd prefer to stay close to the actual api the component is implementing. The component is actually testing an intersection. The issue that came up wasn't an issue of the intersection detector alone, but it interacting with the resize observer.

I think the only possible solution I can think of is a VisibleContainer and a VisibilityDetector which have to be used in combination, as the setup will always be something like this:

container
  > some custom content
    > detector

The issue is that the detector needs to be a child of the custom content, which would increase the complexity of the component. I prefer to stick to the actual functionality of the component as it's clear what you can use it for. If we ever want to have some kind of visibility detector component, it should be separate of course

dataTest,
emptyComponent,
onEndReached,
getOptionClickHandlers,
highlightedOptions,
loading,
renderOption,
options,
selectionHandler,
toggleHighlightedOption,
}) => {
const [remountCounter, setRemountCounter] = useState(0)
const [resizeObserver, setResizeObserver] = useState(null)
const optionsRef = useRef(null)
const wrapperRef = useRef(null)

useEffect(() => {
if (onEndReached && wrapperRef.current) {
// The initial call is irrelevant as there has been
// no resize yet that we want to react to
let firstCall = false

const observer = new ResizeObserver(() => {
if (!firstCall) {
const newCounter = remountCounter + 1
setRemountCounter(newCounter)
firstCall = true
}
})

observer.observe(wrapperRef.current)
setResizeObserver(observer)

return () => observer.disconnect()
}
}, [onEndReached, wrapperRef.current, setRemountCounter])

return (
<div className="optionsContainer">
{loading && (
<div className="loading">
<CircularLoader />
</div>
)}

<div className="container" data-test={dataTest} ref={optionsRef}>
<div className="content-container" ref={wrapperRef}>
{!options.length && emptyComponent}
{options.map(option => {
const highlighted = !!highlightedOptions.find(
highlightedSourceOption =>
highlightedSourceOption === option.value
)

return (
<Fragment key={option.value}>
{renderOption({
...option,
...getOptionClickHandlers(
option,
selectionHandler,
toggleHighlightedOption
),
highlighted,
selected: false,
})}
</Fragment>
)
})}

{onEndReached && resizeObserver && (
<EndIntersectionDetector
dataTest={`${dataTest}-endintersectiondetector`}
key={`key-${remountCounter}`}
rootRef={optionsRef}
onEndReached={onEndReached}
/>
)}
</div>
</div>

<style jsx>{`
.optionsContainer {
flex-grow: 1;
padding: ${spacers.dp4} 0;
position: relative;
overflow: hidden;
}

.container {
overflow-y: auto;
height: 100%;
}

.loading {
display: flex;
height: 100%;
width: 100%;
align-items: center;
justify-content: center;
position: absolute;
z-index: 2;
top: 0;
left: 0;
}

.content-container {
z-index: 1;
position: relative;
}

.loading + .container .content-container {
filter: blur(2px);
}
`}</style>
</div>
)
}

OptionsContainer.propTypes = {
dataTest: propTypes.string.isRequired,
getOptionClickHandlers: propTypes.func.isRequired,
emptyComponent: propTypes.node,
highlightedOptions: propTypes.arrayOf(propTypes.string),
loading: propTypes.bool,
options: propTypes.arrayOf(
propTypes.shape({
label: propTypes.string.isRequired,
value: propTypes.string.isRequired,
})
),
renderOption: propTypes.func,
selectionHandler: propTypes.func,
toggleHighlightedOption: propTypes.func,
onEndReached: propTypes.func,
}
30 changes: 25 additions & 5 deletions packages/widgets/src/Transfer/PickedOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,49 @@
import { spacers } from '@dhis2/ui-constants'
import React from 'react'
import propTypes from '@dhis2/prop-types'

import { spacers } from '@dhis2/ui-constants'
import { EndIntersectionDetector } from './EndIntersectionDetector.js'

export const PickedOptions = ({
children,
dataTest,
selectedEmptyComponent,
pickedOptionsRef,
onPickedEndReached,
}) => (
<div data-test={dataTest}>
{!React.Children.count(children) && selectedEmptyComponent}
{children}
<div className="container" data-test={dataTest} ref={pickedOptionsRef}>
<div className="content-container">
{!React.Children.count(children) && selectedEmptyComponent}
{children}

{onPickedEndReached && (
<EndIntersectionDetector
rootRef={pickedOptionsRef}
onEndReached={onPickedEndReached}
/>
)}
</div>

<style jsx>{`
div {
.container {
padding: ${spacers.dp4} 0;
flex-grow: 1;
overflow-y: auto;
}

.content-container {
position: relative;
}
`}</style>
</div>
)

PickedOptions.propTypes = {
children: propTypes.node.isRequired,
dataTest: propTypes.string.isRequired,
pickedOptionsRef: propTypes.shape({
current: propTypes.instanceOf(HTMLElement),
}),
selectedEmptyComponent: propTypes.node,
onPickedEndReached: propTypes.func,
}
30 changes: 25 additions & 5 deletions packages/widgets/src/Transfer/SourceOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,39 @@
import { spacers } from '@dhis2/ui-constants'
import React from 'react'
import propTypes from '@dhis2/prop-types'

import { spacers } from '@dhis2/ui-constants'
import { EndIntersectionDetector } from './EndIntersectionDetector.js'

export const SourceOptions = ({
children,
dataTest,
sourceEmptyPlaceholder,
Copy link
Copy Markdown

@ghost ghost Jul 30, 2020

Choose a reason for hiding this comment

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

For the PickedOptions this seems to be called selectedEmptyComponent, here it's sourceEmptyPlaceholder. I would expect them to have similar types of names.

Copy link
Copy Markdown
Contributor Author

@Mohammer5 Mohammer5 Jul 30, 2020

Choose a reason for hiding this comment

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

Yes, I've listed some of the inconsistencies in the PR's description that we need to talk about and whether we want to introduce a breaking change

Copy link
Copy Markdown

@ghost ghost Jul 30, 2020

Choose a reason for hiding this comment

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

Ah sorry, I missed that. 👍

sourceOptionsRef,
onSourceEndReached,
}) => (
<div data-test={dataTest}>
{children}
{!React.Children.count(children) && sourceEmptyPlaceholder}
<div className="container" data-test={dataTest} ref={sourceOptionsRef}>
<div className="content-container">
{children}
{!React.Children.count(children) && sourceEmptyPlaceholder}

{onSourceEndReached && (
<EndIntersectionDetector
rootRef={sourceOptionsRef}
onEndReached={onSourceEndReached}
/>
)}
</div>

<style jsx>{`
div {
.container {
padding: ${spacers.dp4} 0;
flex-grow: 1;
overflow-y: auto;
}

.content-container {
position: relative;
}
`}</style>
</div>
)
Expand All @@ -26,4 +42,8 @@ SourceOptions.propTypes = {
dataTest: propTypes.string.isRequired,
children: propTypes.node,
sourceEmptyPlaceholder: propTypes.node,
sourceOptionsRef: propTypes.shape({
current: propTypes.instanceOf(HTMLElement),
}),
onSourceEndReached: propTypes.func,
}
Loading