feat(transfer): add requested improvements#220
Conversation
|
Test summary
Run details
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
changed the title
| export const SourceOptions = ({ | ||
| children, | ||
| dataTest, | ||
| sourceEmptyPlaceholder, |
There was a problem hiding this comment.
For the PickedOptions this seems to be called selectedEmptyComponent, here it's sourceEmptyPlaceholder. I would expect them to have similar types of names.
There was a problem hiding this comment.
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
| const actualFilter = onFilterChange ? searchTerm : internalFilter | ||
| const actualFilterCallback = filterable ? filterCallback : identity | ||
|
|
||
| const [internalFilterPicked, setInternalFilterPicked] = useState( | ||
| initialSearchTermPicked | ||
| ) | ||
| const actualFilterPicked = onFilterPickedChange | ||
| ? searchTermPicked | ||
| : internalFilterPicked | ||
| const actualFilterPickedCallback = filterablePicked | ||
| ? filterCallbackPicked | ||
| : identity |
There was a problem hiding this comment.
It's a bit unclear what's going on in this area.
There was a problem hiding this comment.
I've tried to make the code a bit more clear. Is it better now?
| // observations within a single animation frame. It is benign (your site | ||
| // will not break). | ||
| // | ||
| // Source: https://stackoverflow.com/a/50387233/1319140 |
There was a problem hiding this comment.
I checked out the answer. While it might not be indicative of a problem in the usecase mentioned on stack overflow, it might be in our case? Lea Verou has posted a related issue in the Resize Observer spec repo here that's interesting: WICG/resize-observer#38. I would still be interested in what's causing the error exactly before we ignore it.
There was a problem hiding this comment.
It's working for now. I think we should investigate the exact cause, but for this PR I think it should be fine?
ghost
self-requested a review
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
I noticed it's a bit hard to review these changes since there are multiple issues being addressed in the PR. I know that I often do that myself as well, but as a reviewer it makes it harder to review properly, as it gets a bit confusing. Especially with an intricate component such as this.
|
|
||
| import { EndIntersectionDetector } from './EndIntersectionDetector.js' | ||
|
|
||
| export const OptionsContainer = ({ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Mohammer5
force-pushed
the
Transfer_improvements
branch
from
c0b4b71 to
0cacd43
Compare
|
Regarding the naming convention and possibly renaming props, I don't mind a breaking change to increase the consistency and to make it easier to use / understand. However, as the 2.35 soft freeze is only 2 weeks away and we've had issues with breaking changes in |
Fixes TECH-379
Mohammer5
force-pushed
the
Transfer_improvements
branch
from
1612b91 to
74ff168
Compare
|
🎉 This PR is included in version 5.5.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
4 participants
onSourceEndReached &onPickedEndReached` callbacksloadingSource&loadingPickedflagsRename to
loadingOptions&loadingSelectedfor consistency?option + selectedvssource + pickedvs mixtures of the twosourceEmptyPlaceholder&selectedEmtpyComponentsource&selectedPlaceholder&ComponentPickedSelected?Selected/Pickednot at the end, but in the middle of the prop nameleft/right, notoption/selected(possible confusion once component has been adjusted to adapt to smaller screen sizes)