Skip to content

Use localcontact as default changes on properties - #1050

Merged
skjnldsv merged 1 commit into
masterfrom
fix/localcontact/properties
Apr 14, 2019
Merged

Use localcontact as default changes on properties#1050
skjnldsv merged 1 commit into
masterfrom
fix/localcontact/properties

Conversation

@skjnldsv

@skjnldsv skjnldsv commented Apr 9, 2019

Copy link
Copy Markdown
Member

@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews labels Apr 9, 2019
@skjnldsv skjnldsv added this to the next milestone Apr 9, 2019
@skjnldsv skjnldsv self-assigned this Apr 9, 2019
@skjnldsv
skjnldsv requested a review from caugner April 9, 2019 06:26
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
skjnldsv force-pushed the fix/localcontact/properties branch from a2d5d61 to 6a908b3 Compare April 9, 2019 06:32
@codecov

codecov Bot commented Apr 9, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1050 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1050   +/-   ##
======================================
  Coverage      60%     60%           
======================================
  Files           4       4           
  Lines          60      60           
======================================
  Hits           36      36           
  Misses         24      24

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 6a9025c...6a908b3. Read the comment docs.

@rponline

rponline commented Apr 9, 2019

Copy link
Copy Markdown

Ok, it's working!

The only thing worth mentioning:
The viewport always scrolls on top when deleting a property.

@skjnldsv

skjnldsv commented Apr 9, 2019

Copy link
Copy Markdown
Member Author

@rponline nice catch! :)
We need to prevent the action indeed!

@skjnldsv

skjnldsv commented Apr 10, 2019

Copy link
Copy Markdown
Member Author

@rponline fix is in nextcloud-libraries/nextcloud-vue#342
Let's review and merge this one :)

@skjnldsv
skjnldsv requested a review from rponline April 10, 2019 06:13
@rponline

rponline commented Apr 11, 2019

Copy link
Copy Markdown

I somehow can't use a custom version of nextcloud-vue.

I tried:

  • cloning nextcloud-vue and building it: make build-js
  • Adding the path of the git working directory of nextcloud-vue to the package.json of the contacts app: "nextcloud-vue": "~/git-projects/nextcloud-vue",
  • building the contacts app: make build-js

I also tried make build-js-production and make watch-js ... and deleting package-lock.json .. and node_modules

Is there a howto for setting up a working develeopment environment focusing on npm?

@skjnldsv

skjnldsv commented Apr 11, 2019

Copy link
Copy Markdown
Member Author

@rponline just do make :)
https://github.com/nextcloud/contacts#build-the-app

EDIT: ah sorry, I misread, you're talking about nextcloud-vue!

  1. on the git cloned directory of nextcloud-vue, do make then npm link
  2. on the contacts repo do: npm install then npm link nextcloud-vue
  3. on the contacts repo still: make build-js or make build-js-production

Or manually copy the dist folder from nextcloud-vue onto the contacts node_modules/nextcloud-vue folder (and erase existing files)

@rponline

Copy link
Copy Markdown

I can't test this for reasons I don't understand.

after using npm link, the contacts JS is not compiling anymore:
image

Without using npm link (and just copying the dist/ content), js compiles, but the contacts app is not working anymore: It just stops doing stuff and shows a blank page:

image

Same behavior when i just use the master branch of nextcloud-vue ...
I don't know what's wrong with my environment or what I'm doing wrong.

Someone else should test it instead, please :) (@caugner ?)

@skjnldsv

Copy link
Copy Markdown
Member Author

Hum, I had that once.
Ignore it, yo can still test this pr without the nextcloud-vue part.

Just ignore the viewport issue, i'll be fixed automatically once we released a new nextcloud-vue release :)

@skjnldsv
skjnldsv merged commit abab5c5 into master Apr 14, 2019
@skjnldsv
skjnldsv deleted the fix/localcontact/properties branch April 14, 2019 06:13
@rponline

Copy link
Copy Markdown

Ignoring the whole scrolling issue:
yes, it's working :)

@skjnldsv skjnldsv modified the milestones: next, 3.1.1 Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New contact: TEL, EMAIL and ADR cannot be removed together

2 participants