Skip to content
This repository has been archived by the owner on May 20, 2024. It is now read-only.

fix(activities): only hover pictures if device supports it #2297

Merged
merged 5 commits into from
Jan 30, 2021
Merged

fix(activities): only hover pictures if device supports it #2297

merged 5 commits into from
Jan 30, 2021

Conversation

larzon83
Copy link
Contributor

@larzon83 larzon83 commented Jan 28, 2021

Closes #2257

What does this PR do?

Optimization of this PR: #2258

UserSlot.vue:

  • add @media (hover) to enable hover if device supports it
  • remove hoverHide div and its style as it's not needed
  • rename hoverUser prop to "user" for better readability
  • rename hoverShow class to "show-picture-on-hover"
  • fix wrong style declaration cursor ini in class greyedOut -> now it's cursor default as these boxes should not be clickable

EmptySlot.vue:

  • fix wrong style declaration cursor ini in class greyedOut -> now it's cursor default as these boxes should not be clickable
  • add @click.stop to stop going to activity detail when clicking on an empty slot

CurrentUser.vue:

  • add @media (hover) to enable hover if device supports it (the leave icon in this case)

Links to related issues

Checklist

  • added a test, or explain why one is not needed/possible...
  • no unrelated changes
  • asked someone for a code review
  • joined chat.foodsaving.world/channel/karrot-dev
  • added an entry to CHANGELOG.md (description, pull request link, username(s))

larzon83 and others added 3 commits January 22, 2021 23:23
changed: use default group location for defaultMapCenter when creatin…
UserSlot.vue:
- add "@media (hover)" to enable hover if device supports it
- remove "hoverHide" div and its style as it's not needed
- rename "hoverUser" prop to "user" for better readability
- rename "hoverShow" class to "show-picture-on-hover"
- fix wrong style declaration "cursor ini" in class "greyedOut" -> now it's "cursor default" as these boxes should not be clickable

EmptySlot.vue:
- fix wrong style declaration "cursor ini" in class "greyedOut" -> now it's "cursor default" as these boxes should not be clickable

CurrentUser.vue:
- add "@media (hover)" to enable hover if device supports it (the leave icon in this case)
Copy link
Member

@nicksellen nicksellen left a comment

Choose a reason for hiding this comment

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

Thanks!

I really appreciate the small naming/style changes, and the CSS fixes. Cleaning up and tidying as you go! I like it 👍

Also very happy as it seems like it'll fix the issue nicely! I had got kind of stuck on it 🚀

My only real change request is the comment, but also if you can check why the event is propagating when it looks like it shouldn't (to my eyes) that would be great! ⭐

To get the tests to pass, it's the snapshot tests that are failing, I can explain more somewhere else, but for now if you just run yarn test -u, and then commit the snapshot file that will have changed, that'll cause the tests to pass.

Regarding #2258 (comment):

Could you do another deploy of my PR?

The deployments are automatic, but only for branches inside our repo (i.e. not for your PR because it's from an external repo), easiest/best thing is we add you to "Karrot Devs" group and you can work on branches in our repo directly, and get branch deployments (see my message in chat for more info).

src/activities/components/ActivityItem.vue Outdated Show resolved Hide resolved
src/activities/components/UserSlot.vue Outdated Show resolved Hide resolved
@larzon83
Copy link
Contributor Author

I will install yarn and check again. I'll update tests too then.
As I am now a member of the repo: should we continue this PR from my fork or shall I create a new one directly in the repo so we can have branch deploys?

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #2297 (57a5d8f) into master (b0202e1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2297   +/-   ##
=======================================
  Coverage   57.34%   57.34%           
=======================================
  Files         286      286           
  Lines        5788     5788           
  Branches      911      911           
=======================================
  Hits         3319     3319           
  Misses       2466     2466           
  Partials        3        3           
Impacted Files Coverage Δ
src/activities/components/ActivityUsers.vue 100.00% <ø> (ø)
src/activities/components/CurrentUser.vue 100.00% <ø> (ø)
src/activities/components/EmptySlot.vue 100.00% <ø> (ø)
src/activities/components/UserSlot.vue 100.00% <ø> (ø)
src/places/components/PlaceEdit.vue 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0202e1...57a5d8f. Read the comment docs.

@larzon83
Copy link
Contributor Author

@nicksellen Updated snapshot. Also I have installed yarn and did all fresh. With yarn dev, it also runs fine with localhost in iOS Simulator.

Copy link
Member

@nicksellen nicksellen left a comment

Choose a reason for hiding this comment

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

Looks good now! Happy for a merge 🚀

@nicksellen nicksellen merged commit 0a77e11 into karrot-dev:master Jan 30, 2021
@larzon83 larzon83 deleted the fix/hover-activities branch January 30, 2021 12:22
@larzon83
Copy link
Contributor Author

https://dev.karrot.world now works fine on my iPhone 😄

@larzon83 larzon83 mentioned this pull request Jan 30, 2021
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joining activity on Iphone requires two tapings before getting confirmation dialog
2 participants