Skip to content
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.

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

chore: open ContextMenu on right-click #929

Merged
merged 5 commits into from
Jan 15, 2019

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented Jan 8, 2019

The ContextMenu was a standalone component but to add the ability to be opened by right-clicking a file I had to refactor it a bit and couple its logic to the File component.

Right-clicking a file now opens the context menu, but always in the default position. I've tried to open it where the user clicked but that was taking a lot of time and I don't see this as a priority right now.

Closes #917.

@fsdiogo fsdiogo requested review from olizilla and hacdias January 8, 2019 16:08
@ghost ghost assigned fsdiogo Jan 8, 2019
@ghost ghost added the status/in-progress In progress label Jan 8, 2019
@hacdias
Copy link
Member

hacdias commented Jan 8, 2019

Hmm... I like the idea of opening it with right-click but I believe it's not a good UX not to open it where we clicked. It was completely unexpected when the menu opened in the other end of the screen. Usually our instinct tells us to right click on the icon or name. I think we should give this a bit more work to open the place we click.

I found two packages that seem helpful:

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 9, 2019

I like the idea of opening it with right-click but I believe it's not a good UX not to open it where we clicked.

I 💯 agree. I'll give this another stab today!

@fsdiogo fsdiogo changed the title chore: open ContextMenu on right-click [WIP] chore: open ContextMenu on right-click Jan 9, 2019
@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 10, 2019

The last commit opens the ContextMenu where the user right-clicked:

ctx

TODO:

@fsdiogo fsdiogo changed the title [WIP] chore: open ContextMenu on right-click chore: open ContextMenu on right-click Jan 11, 2019
@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 11, 2019

This is ready for review! 🌟

@hacdias
Copy link
Member

hacdias commented Jan 11, 2019

@fsdiogo I tried to right click and got this:

image

I made sure the dependencies were up to date. It seems that contextMenu is not being set.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 14, 2019

Hmm, just tested on Firefox and that error came up. It works on Chrome and Safari.

I'll look into it!

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 14, 2019

Fixed!

@hacdias
Copy link
Member

hacdias commented Jan 14, 2019

Just as a FYI, I used Chrome too, not FF. Lemme check.

@hacdias
Copy link
Member

hacdias commented Jan 14, 2019

@fsdiogo working here and looking good. The CI is failing though both on build and navigation. :/

@olizilla
Copy link
Member

it's a dependencies issue

> ipfs-webui@2.3.2 build:darwin:linux /home/travis/build/ipfs-shipyard/ipfs-webui
> cross-env REACT_APP_GIT_REV=`git rev-parse --short HEAD` react-scripts build
Creating an optimized production build...
Failed to compile.
Module not found: Error: Can't resolve 'webcrypto-shim' in '/home/travis/build/ipfs-shipyard/ipfs-webui/node_modules/libp2p-crypto/src'

@olizilla
Copy link
Member

@hacdias @fsdiogo can you confirm what versions of npm you are using pls

@hacdias
Copy link
Member

hacdias commented Jan 14, 2019

Latest: 6.5.0

@olizilla
Copy link
Member

reverting the unrelated changes to the package-lock.json fixes the error, so I'm going to push that change to this branch.

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost assigned olizilla Jan 14, 2019
@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 15, 2019

Can you take a look at the code so we can merge this?

We should release a new version of Web UI ASAP to fix the add files regression.

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM 😄

Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

👌 neat change and a good tidy up!

@fsdiogo fsdiogo merged commit 85c9ac3 into master Jan 15, 2019
@ghost ghost removed the status/in-progress In progress label Jan 15, 2019
@fsdiogo fsdiogo deleted the chore/open-ctx-menu-on-right-click branch January 15, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants