joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Feature reorder projects #375

Closed AWolf81 closed 5 years ago

AWolf81 commented 5 years ago

Based on branch from @beyersito (see PR #310).

I'm comandeering this to get this ready. I think this is ready for review - I just need to find the offset issue that occurs if icon is dragged with a scrolled sidebar (see third screenrecording)

Related Issue:

195

Summary:

Screenshots/GIFs: Drag & drop with fullscreen window Screenrecording_reorder_projects_fullscreen

Drag & drop reduced window size (not scrolled) Screenrecording_reorder_projects_reduced size_dnd_not_scrolled

Drag & drop reduced window size (scrolled) - y-offset issue Screenrecording_reorder_projects_reduced size_dnd_scrolled

Todo

codecov[bot] commented 5 years ago

Codecov Report

Merging #375 into master will increase coverage by 0.86%. The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #375      +/-   ##
=========================================
+ Coverage   57.94%   58.8%   +0.86%     
=========================================
  Files         158     158              
  Lines        3331    3362      +31     
  Branches      462     468       +6     
=========================================
+ Hits         1930    1977      +47     
+ Misses       1195    1181      -14     
+ Partials      206     204       -2
Impacted Files Coverage Ξ”
src/components/App/App.js 0% <0%> (ΓΈ) :arrow_up:
src/components/Sidebar/SidebarProjectIcon.js 100% <100%> (+20%) :arrow_up:
src/actions/index.js 94.02% <66.66%> (-0.63%) :arrow_down:
src/components/Sidebar/Sidebar.js 97.5% <92.3%> (-2.5%) :arrow_down:
src/reducers/projects.reducer.js 89.02% <95.23%> (+31.21%) :arrow_up:
melanieseltzer commented 5 years ago

[Edit] Animation was needed, using portal fix instead

~I just did some research and looks like people were having the same offset issues, and it was due to a parent node that had the transform property. Sure enough when I removed the offset and all the animated stuff on Wrapper the issue went away. See https://github.com/atlassian/react-beautiful-dnd/issues/499#issuecomment-389724270~

~I pushed up the fix but I'm not sure why we were using animation on the wrapper, can you take a look? Not sure if animation was ultimately necessary and if it is feel free to discard my commit if it's not a good fix :)~

AWolf81 commented 5 years ago

@melanieseltzer πŸ‘ wow, great. For me that's OK. I'm not sure why there was an animation on the y-offset. Maybe it was a slide-in animation but I haven't seen that.

I think it's OK to remove. If there was an anmiation we could add it later.

melanieseltzer commented 5 years ago

@AWolf81 Turns out animation is needed for the sidebar slide in, so instead I've just implemented a portal which is the recommended fix in the original issue I posted above. I removed the previous commit since I needed to undo it - can you make sure this portal fix is working for you? Looks good my end.

AWolf81 commented 5 years ago

I think I'll have to do some debugging later today because there are two things I'd like to check:

  1. Added divs at end of body (two more divs after each drag&drop) & scrollbars (see screenshot below)
  2. Dragging sometimes not working as the cursor not changing to a finger (could be related to 1. or I reduced the screen height too much)

grafik

melanieseltzer commented 5 years ago

Good catch, I think it was due to being in the render so I moved the portal creation outside, does that fix things? (not sure about the scrollbars because I don't see them but maybe it was all related).

AWolf81 commented 5 years ago

OK, the change of the portal location fixed the problem. There is just one small issue left but I think that's OK to tackle after this PR is merged. The Infobar of OnlineComponent is preventing dragging in Sidebar. grafik

To the mentioned portal handling: I think it's OK to add the portal to body from Sidebar component. Maybe we could add an id like sidebarDndPortal so we know that this portal is used from the Sidebar component. But I think that optional. I played a bit with portals in this Codesandbox.

melanieseltzer commented 5 years ago

Good idea, just added the id πŸ‘ In terms of extracting the portal to its own location, I think we could wait until we need to reuse it somewhere else since it's a pretty specific fix for a niche issue.

melanieseltzer commented 5 years ago

@AWolf81 i'll extract that issue with Infobar into a separate issue for cleanup. I think that's a Windows specific thing because I don't have the problem.