Export contact and addressbook as vcard + header restructuration fix #349

skjnldsv · 2016-04-10T09:41:59Z

Fixes #247 and #248
Waiting for owncloud/core#23891 for the white download icons

Add option to export an addressbook with all its contacts
Add option to export a single contact as a vCard

mention-bot · 2016-04-10T09:42:00Z

By analyzing the blame information on this pull request, we identified @jancborchardt, @Henni and @DeepDiver1975 to be potential reviewers

codecov-io · 2016-04-10T09:46:21Z

Current coverage is `11.05%`

Merging #349 into master will decrease coverage by -0.01% as of 9dc77d5

@@            master    #349   diff @@
======================================
  Files           42      42       
  Stmts          741     742     +1
  Branches         0       0       
  Methods          0       0       
======================================
  Hit             82      82       
  Partial          0       0       
- Missed         659     660     +1

Review entire Coverage Diff as of 9dc77d5

Uncovered Suggestions

+2.02% via ...dressBook_service.js#148...162
+1.61% via ...tImport_directive.js#3...14
+1.61% via ...etails_controller.js#47...58
See 7 more...

Powered by Codecov. Updated on successful CI builds.

jancborchardt · 2016-04-10T11:55:07Z

The icons are too close together and look a bit strange on top of each other. I would place them left/right next to each other, with the delete icon on the right.

skjnldsv · 2016-04-10T11:56:37Z

Like this?

jancborchardt · 2016-04-10T11:57:23Z

Yes, but with more padding. The clickable area of an icon needs to be 44*44px at least.

skjnldsv · 2016-04-10T11:59:11Z

Okay, and what about the mobile view?
It break a lot of stuff.
I think we need to fix the header, using position absolute in css isn't really a good idea for media queries.

jancborchardt · 2016-04-10T12:13:11Z

On mobile it could be shown above each other, it’s the less important view. But on desktop it should look proper.

I think we need to fix the header, using position absolute in css isn't really a good idea for media queries.

What do you mean by that? The name should always be shown fixed? I would say not because then you have very little space for typing in the fields, when the keyboard is also expanded.

skjnldsv · 2016-04-10T12:19:40Z

I mean, in css, you need to keep a structure.
Using the absolute position in css should only be used for elements that break the cascading structure of your page. In this case, the h2, inputs, buttons, don't break this at all.
We should use either a flexbox or a float, but using absolute break many further possibilities to make a dynamic design, because the element become an independent item and isn't related or linked to elements around it.

This is a good example, the header become broken in mobile view. With a good fluid css design, the elements should at least rearrange to display themselves in the page. Because of the absolute we got div out of the view! Not a good idea!

Unfortunately, a lot of design part in owncloud use the position absolute option and I don't think this is very convenient. :)

DeepDiver1975 · 2016-04-10T19:16:37Z

We have the recurring issue that this is not working with 9.0.0 and 9.0.1 because the icon is not in core.
Please add the download icon to the app and use this one until we drop support for 9.0.0 and 9.0.1 (assuming we backport owncloud/core#23891 )

skjnldsv · 2016-04-11T02:33:35Z

Okay, I wasn't sure how to proceed! Thanks Deep! :)

jancborchardt · 2016-04-11T11:39:50Z

@skjnldsv actually the absolute positioning in combination with the correct sizing (width percentage), testing and additional responsive rules works very well everywhere. Floats on the other hand are very difficult to control. Most of the time our layout is simple anyway (and I intend to keep it that way), with 3 elements max next to each other, so absolute positioning is fine.
Hope that explains it? :)

Henni · 2016-04-11T11:40:48Z

@skjnldsv I suggest that this PR should only introduce the export of a single contact.
Let's leave the export function for addressbooks to a different PR as it seems to be a bigger task.

jancborchardt · 2016-04-11T11:53:44Z

Yeah, however the export of the whole address book has higher priority for sure. I would say it should be done first.

skjnldsv · 2016-04-11T14:08:04Z

@Henni this is a single link for the addressbook too. With just a condition to check the dav lib version.
Shouldn't be that hard.

@jancborchardt no, absolute positioning is not fine at all. It works because it's correctly written, but it's terrible coding for css! :p
It's my humble opinion. And for the float, it's hard to master, but works well (even if now flex is faaar better for positioning and alignement, and should put float on the bench.). :)

For the priority I worked for things that was possible, I couldn't do the addressbook export as long as we didn't have the possibility. Now we do, commit on his way.

Edit: sorry if I sound presumptuous, this isn't on purpose at all, just my opinion (bad or good, you decide) (and i also just came back from work and I'm exhausted ;) )

DeepDiver1975 · 2016-04-12T07:42:01Z

With just a condition to check the dav lib version.
Shouldn't be that hard.

If I remember correctly we have no mechanism on the js side to read the version number ....

DeepDiver1975 · 2016-04-12T07:44:11Z

wait ... oc_config can be used ...

skjnldsv · 2016-04-12T11:30:07Z

Thanks @DeepDiver1975 👍

skjnldsv · 2016-04-16T07:54:20Z

@DeepDiver1975 ok, seems to work. I don't really like using the controller as it seems easier to pass the whole thing into a ng-shown but parseFloat isn't declared into $scope so I can't use it, it occurred to me it was simpler this way. What do you think? Is this over complicated?

@jancborchardt I will update the header later :)

skjnldsv · 2016-04-16T20:08:08Z

@jancborchardt I fixed the whole header. This is now more flexible and cross-device.
This is what growing and shrinking looks like:

ps: because there is no profile picture, the missing space is ok. This should be fixed when the upload form will be done by Henni.

jancborchardt · 2016-04-17T14:22:50Z

@skjnldsv I do like the restructuring except for the removal of the left whitespace. On the most narrow width it’s ok, but on the other widths it’s intentional to line up with the field contents.

skjnldsv · 2016-04-17T14:26:27Z

The left space is because i'm missing the div that replace the img in #342 :)
I can add it if you want?

jancborchardt · 2016-04-17T22:12:54Z

Yeah, it should be added please – it gives better visual balance. You can remove it on the most narrow view if you like, but we need to show the avatar in some way there anyway.

skjnldsv · 2016-04-18T06:14:40Z

No no, I agree with you! :)

skjnldsv · 2016-04-18T07:36:49Z

Rebased and ready to go!

DeepDiver1975 · 2016-04-18T07:59:37Z

js/components/addressBook/addressBook_controller.js

 	ctrl.showUrl = false;
+	/* globals oc_config */
+	/* eslint camelcase: [2, {properties: "never"}] */
+	ctrl.canExport = parseFloat(oc_config.version.substring(0, 3)) >= 9.1;


float is not really the right thing to use - generally speaking since 9.1 and 9.10 are equal in terms of float but not in terms of version. Anyway - there will never be a version higher then 9.2.

BUT: 9.1 is not the correct value to test against. you need to test against 9.0.2 because the fix has been backported

Oh okay! Thanks! :)
What should I do then?

skjnldsv · 2016-04-18T08:55:29Z

@DeepDiver1975 rebased and fixed.

DeepDiver1975 · 2016-04-18T19:29:11Z

templates/addressBook.html

 <span class="addressBookName">{{ctrl.addressBook.displayName}}</span>
 <span class="utils">
+	<span class="action" ng-show="ctrl.canExport">
+		<span href="{{ctrl.addressBook.url}}?export"


span? Doesn't work for me ....

Do you have the version > 9.0.2?
It works fine here, I don't understand. What's wrong?

... No comment, I didn't realized! ^^
I must have left some testing.

Thanks for the commit.

Added addressbook export button Fixed header css and export button location + add empty div to replace empty space if no picture

skjnldsv · 2016-04-20T05:56:31Z

Rebased and waiting for tests before merging.

jancborchardt · 2016-04-20T07:43:59Z

Nice, very cool!

skjnldsv added enhancement 2 - developing in progress labels Apr 10, 2016

DeepDiver1975 added this to the 1.2 milestone Apr 10, 2016

DeepDiver1975 force-pushed the add-export-buttons branch from 946085d to 4cf0832 Compare April 10, 2016 19:17

skjnldsv force-pushed the add-export-buttons branch from 4cf0832 to ac1fd37 Compare April 16, 2016 07:51

skjnldsv changed the title ~~Export contact and addressbook as vcard~~ Export contact and addressbook as vcard + header restructuration fix Apr 16, 2016

skjnldsv force-pushed the add-export-buttons branch 2 times, most recently from 0c3f5ed to 69cfdd4 Compare April 18, 2016 07:35

skjnldsv added 3 - to review and removed 2 - developing in progress labels Apr 18, 2016

skjnldsv force-pushed the add-export-buttons branch from 69cfdd4 to 44aa0ec Compare April 18, 2016 07:36

DeepDiver1975 reviewed Apr 18, 2016
View reviewed changes

skjnldsv force-pushed the add-export-buttons branch from 45867df to 81afb34 Compare April 18, 2016 08:55

DeepDiver1975 reviewed Apr 18, 2016
View reviewed changes

skjnldsv and others added 2 commits April 20, 2016 07:55

Single contact export button

0275c39

Added addressbook export button Fixed header css and export button location + add empty div to replace empty space if no picture

Fix address book download

36aba55

skjnldsv force-pushed the add-export-buttons branch from 6d0efb0 to 36aba55 Compare April 20, 2016 05:55

skjnldsv self-assigned this Apr 20, 2016

skjnldsv added 4 - to release and removed 3 - to review labels Apr 20, 2016

skjnldsv merged commit 0be9900 into master Apr 20, 2016

skjnldsv deleted the add-export-buttons branch April 20, 2016 06:00

Export contact and addressbook as vcard + header restructuration fix #349

Export contact and addressbook as vcard + header restructuration fix #349

Uh oh!

Conversation

skjnldsv commented Apr 10, 2016

Uh oh!

mention-bot commented Apr 10, 2016

Uh oh!

codecov-io commented Apr 10, 2016

Current coverage is 11.05%

Uncovered Suggestions

Uh oh!

jancborchardt commented Apr 10, 2016

Uh oh!

skjnldsv commented Apr 10, 2016

Uh oh!

jancborchardt commented Apr 10, 2016

Uh oh!

skjnldsv commented Apr 10, 2016

Uh oh!

jancborchardt commented Apr 10, 2016

Uh oh!

skjnldsv commented Apr 10, 2016

Uh oh!

DeepDiver1975 commented Apr 10, 2016

Uh oh!

skjnldsv commented Apr 11, 2016

Uh oh!

jancborchardt commented Apr 11, 2016

Uh oh!

Henni commented Apr 11, 2016

Uh oh!

jancborchardt commented Apr 11, 2016

Uh oh!

skjnldsv commented Apr 11, 2016

Uh oh!

DeepDiver1975 commented Apr 12, 2016

Uh oh!

DeepDiver1975 commented Apr 12, 2016

Uh oh!

skjnldsv commented Apr 12, 2016

Uh oh!

skjnldsv commented Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt commented Apr 17, 2016

Uh oh!

skjnldsv commented Apr 17, 2016

Uh oh!

jancborchardt commented Apr 17, 2016

Uh oh!

skjnldsv commented Apr 18, 2016

Uh oh!

skjnldsv commented Apr 18, 2016

Uh oh!

DeepDiver1975 Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Apr 18, 2016

Uh oh!

DeepDiver1975 Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Apr 20, 2016

Uh oh!

jancborchardt commented Apr 20, 2016

Uh oh!

Current coverage is `11.05%`

skjnldsv commented Apr 16, 2016 •

edited

Loading

skjnldsv commented Apr 16, 2016 •

edited

Loading

skjnldsv Apr 20, 2016 •

edited

Loading