-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add visual cue when moving draggable item over droppable item, #20150
Add visual cue when moving draggable item over droppable item, #20150
Conversation
very nice. thanks for the pull request :-) |
@pellaeon I can't find any visual changes, when testing with chrome. Can you maybe add some screenshots? Maybe I just tried the wrong stuff ;) |
@MorrisJobke |
@owncloud/designers |
@jancborchardt do you like this highlight ? |
The color of the highlight should be the same that is used for when an upload is successfully finished. And that shade of yellow should be defined centrally so we can pull it as highlightcolor and change it in one place. |
It is the same color, copied from https://github.com/owncloud/core/blob/master/apps/files/css/upload.css#L171 . I don't know how to define it centrally. |
Tested, functionally it works. |
Yeah, the style should cover the whole row. We can adjust the exact color later. |
@pellaeon can you change it to cover the whole row ? |
7079240
to
c07d43a
Compare
@PVince81 it now covers the whole row, also making the whole row droppable. |
dafff84
to
d8969bd
Compare
Looks good to me, JS unit tests pass locally 👍 |
This has conflicts :( Generally I like this, but maybe we want to use a not that aggressive color. But that could be addressed in an additional PR. cc @owncloud/designers @pellaeon Could you rebase and resolve the conflicts? Then we could merge this :) |
ie. breadcrumb and filenameTd
so that .canDrop will highlight the entire row when drag hover
d8969bd
to
56241af
Compare
@MorrisJobke rebased! |
Ok, apparently, my stripped background problem is a webkit issue. It work fine on other browsers. |
@skjnldsv I'd rather vote against it, at least for now. Maybe reconsider striping all the things for 9.2 ? Goal is to get this PR here merged soon. |
@PVince81 Yep, let's merge! :) |
@skjnldsv counting your comment as a 👍 |
JS unit tests still passed locally, merging |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
ie. breadcrumb and filenameTd
The style on breadcrumb might need tweaking.
Solves #10369