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

Sharing UI #952

Merged
merged 20 commits into from
Apr 6, 2020
Merged

Sharing UI #952

merged 20 commits into from
Apr 6, 2020

Conversation

marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Mar 28, 2020

Sharing user:
Screenshot from 2020-03-31 14-39-24
Screenshot from 2020-03-31 14-40-39

Sharee:

Screenshot from 2020-03-31 14-40-00

Public link
Screenshot from 2020-03-31 14-43-03

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Wow, this is very very cool! My only feedback is kinda unrelated. :D

  • On the sharing page, the title subline is black but should be white
  • Instead of breadbrumbs, we should use the system that Photos uses with the nice large headings

So 👍 from me

@skjnldsv
Copy link
Member

  • Instead of breadbrumbs, we should use the system that Photos uses with the nice large headings

I think there is an issue already.
Let's stay relevant to this issue ;)

@jancborchardt
Copy link
Member

  • Instead of breadbrumbs, we should use the system that Photos uses with the nice large headings

I think there is an issue already.
Let's stay relevant to this issue ;)

Exactly why I simply gave my 👍 and said it’s unrelated. ;)

@marcelklehr
Copy link
Member Author

Ah, nice I hadn't seen the photos app approach, yet :)

On the sharing page, the title subline is black but should be white

I was baffled by that as well. I'm using the PublicPageTemplate to set those, so I wasn't sure whether I should style that myself, or if this is an upstream bug.

@skjnldsv
Copy link
Member

Please rebase ;)
Tests should be running now, I feel more confident reviewing this pr now :)

@marcelklehr
Copy link
Member Author

The server-side tests are broken, atm, I will work on this laters

@marcelklehr
Copy link
Member Author

I should get those complexity numbers down... 😅

@jancborchardt
Copy link
Member

So @skjnldsv will be able to talk about this more, but the Sharing UI would be a good candidate for a component in the Vue components library :) https://github.com/nextcloud/nextcloud-vue

@codecov-io
Copy link

Codecov Report

Merging #952 into master will decrease coverage by 1.10%.
The diff coverage is 13.70%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #952      +/-   ##
============================================
- Coverage     41.81%   40.70%   -1.11%     
- Complexity      764      785      +21     
============================================
  Files            51       51              
  Lines          2810     2874      +64     
============================================
- Hits           1175     1170       -5     
- Misses         1635     1704      +69     
Impacted Files Coverage Δ Complexity Δ
appinfo/routes.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
lib/AppInfo/Application.php 0.00% <0.00%> (ø) 2.00 <0.00> (ø)
lib/Controller/InternalFoldersController.php 0.00% <0.00%> (ø) 17.00 <0.00> (ø)
lib/Controller/WebViewController.php 0.00% <0.00%> (ø) 6.00 <5.00> (+4.00)
lib/Db/ShareMapper.php 29.50% <ø> (-9.84%) 10.00 <0.00> (ø)
...b/Migration/Version003000000Date20191123094721.php 0.00% <0.00%> (ø) 16.00 <0.00> (+12.00)
...b/Migration/Version003000000Date20191129094721.php 0.00% <ø> (ø) 4.00 <0.00> (-5.00)
lib/Service/BookmarkPreviewer.php 0.00% <0.00%> (ø) 6.00 <0.00> (ø)
lib/Controller/FoldersController.php 45.29% <38.77%> (-2.88%) 132.00 <11.00> (+4.00) ⬇️
lib/Controller/BookmarkController.php 38.57% <50.00%> (+0.40%) 126.00 <0.00> (-1.00) ⬆️
... and 4 more

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 ef88b8c...834a8ba. Read the comment docs.

@marcelklehr
Copy link
Member Author

@skjnldsv This is good to review now :)

@marcelklehr marcelklehr merged commit 834a8ba into nextcloud:master Apr 6, 2020
@ccoenen
Copy link

ccoenen commented Apr 6, 2020

omg-its-happening.gif here. Thanks everyone! Looking forward to the release!

@jancborchardt
Copy link
Member

Props @marcelklehr, really good work! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants