Skip to content

Conversation

@tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Jun 6, 2018

Based on #2685

TODO

  • enter a new comment
    • refreshes activity stream
    • or shows snackbar with error

device-2018-06-06-112600

@tobiasKaminsky tobiasKaminsky changed the base branch from master to versions June 6, 2018 06:28
@AndyScherzinger
Copy link
Member

I'll take care of the button 👍


Bundle bundle = getArguments();
setFile((OCFile) bundle.getParcelable(EXTRA_FILE));
setFile(bundle.getParcelable(EXTRA_FILE));
Copy link
Member Author

Choose a reason for hiding this comment

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

I accidentally touched the file and now AndroidStudio always tries to enhance this…

@AndyScherzinger
Copy link
Member

@tobiasKaminsky button has been beautified, see c2c338a

@AndyScherzinger
Copy link
Member

Edit Text tinting has been added via d302e3f

@AndyScherzinger AndyScherzinger changed the title Comments Iteration 4 for File detail & new sharing - Comments Jun 6, 2018
@AndyScherzinger
Copy link
Member

@tobiasKaminsky except for the rebase/conflict-resolving is there anything left open for this PR since I did the 2 remaining TODOs you wrote down?

@tobiasKaminsky
Copy link
Member Author

Same as the other PR, once this is ready an versions PR is merged, we need do rebase this.

@tobiasKaminsky
Copy link
Member Author

This is now ready for review 👍

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jun 7, 2018

👍 tested and works fine for me 👍

Approved with PullApprove

@tobiasKaminsky
Copy link
Member Author

Thanks for testing 👍
Regarding the upcoming merge, I suggest, that we merge #2685, and then I rebase this against master and then we merge this like any other regular PR.

@mario
Copy link
Contributor

mario commented Jun 10, 2018

Conflicts + submit button should be rotated by 45 degrees to the left.

@AndyScherzinger
Copy link
Member

submit button should be rotated by 45 degrees to the left.

isn't Talk the only app doing this? 😝 (looking at mail/messengers/etc.)

@mario
Copy link
Contributor

mario commented Jun 10, 2018

@AndyScherzinger yes. Consistency :P

private boolean isSuccess(int status) {
return status == HttpStatus.SC_CREATED;
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

New line.

@@ -0,0 +1,112 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Whole file should be in a library.

Copy link
Member

Choose a reason for hiding this comment

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

General issue: ATM all "operations" are afaik not in the lib but the app...

Copy link
Contributor

Choose a reason for hiding this comment

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

General issue maybe, but at least move this NEW issue to the library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently this cannot be moved this easily as these *Operation extends RemoteOperation which is a class in files app.

I guess you confuse it with *RemoteOperations, e.g.:

RemoveFileOperation is in Files app (handles app specific stuff)
RemoveRemoteFileOperation is in library (and handles the real webdav call)

@AndyScherzinger AndyScherzinger changed the title Iteration 4 for File detail & new sharing - Comments Iteration 5 for File detail & new sharing - Comments Jun 12, 2018
@tobiasKaminsky
Copy link
Member Author

submit button should be rotated by 45 degrees to the left.

isn't Talk the only app doing this? 😝 (looking at mail/messengers/etc.)

Having a look at material icons (https://material.io/tools/icons/?icon=send&style=baseline)
image

and our web ui:
image

We should rather change the icon in talk, to stay consistent.
@nextcloud/designers

@tobiasKaminsky
Copy link
Member Author

! Rebased

@nextcloud nextcloud deleted a comment Jun 14, 2018
@nextcloud nextcloud deleted a comment Jun 14, 2018
@jancborchardt
Copy link
Member

@tobiasKaminsky yup – the icon you show is correct. Looking at Signal they do the same.

  • The first line of the comment »Commented by you« is not really useful. It should simply say »You« instead
  • Similar for the icon: Instead of the generic comment icon, it should show your avatar
  • And the comment text below should have the same text size as the normal text. We shouldn’t have too many different text sizes as that looks strange.
  • We can remove the »Today« header – it’s not necessary to show that one. Should only start with »Yesterday«.

Also cc @nickvergessen cause I guess these things have to do with notifications?

@AndyScherzinger
Copy link
Member

The first line of the comment »Commented by you« is not really useful. It should simply say »You« instead

That is the content we get from the server, so @nickvergessen or somebody else would have to change that in the server.

@mario
Copy link
Contributor

mario commented Jun 14, 2018

👍

Approved with PullApprove

@tobiasKaminsky
Copy link
Member Author

@jancborchardt this is all from server side, except of the date header "today, yesterday, …", which is the same as the activity stream.

Please do not merge this, as this would then be merged in versions branch.

@tobiasKaminsky tobiasKaminsky changed the base branch from versions to master June 15, 2018 09:58
tobiasKaminsky and others added 7 commits June 20, 2018 11:14
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@codecov
Copy link

codecov bot commented Jun 20, 2018

Codecov Report

Merging #2690 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2690      +/-   ##
=========================================
- Coverage    6.49%   6.48%   -0.01%     
=========================================
  Files         287     288       +1     
  Lines       29058   29108      +50     
  Branches     4234    4235       +1     
=========================================
+ Hits         1888    1889       +1     
- Misses      26886   26935      +49     
  Partials      284     284
Impacted Files Coverage Δ
...cloud/android/operations/CommentFileOperation.java 0% <0%> (ø)
...roid/ui/fragment/FileDetailActivitiesFragment.java 0% <0%> (ø) ⬆️
...com/owncloud/android/ui/fragment/FileFragment.java 0% <0%> (ø) ⬆️
.../third_parties/daveKoeller/AlphanumComparator.java 82.75% <0%> (+1.14%) ⬆️

@AndyScherzinger
Copy link
Member

Please do not merge this, as this would then be merged in versions branch.

@tobiasKaminsky you rebased the branch and versions has been merged, so I'd say the PR can be merged now, right? :)

@jancborchardt
Copy link
Member

@tobiasKaminsky ok, will open in Activity then.

But text size is from Android also, right? Can you adjust that as I mentioned above?

@tobiasKaminsky
Copy link
Member Author

tobiasKaminsky commented Jun 20, 2018

@jancborchardt I will do it in a following PR / discuss in a new issue.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants