Add expiration date to share by link view #19513
Conversation
|
nice small improvement 👍 |
There was a problem hiding this comment.
I just copied from filelist.js 🙈
|
@phil-davis @PVince81 I addressed your comments:
|
|
will review/test after the 8.2 rush... |
|
Another problem that I noticed during working on this: Sorting is handled in the FileList in a generic way. But it has one combined attribute to "How to sort" (time based) and "What to sort" (share time). Currently it only has sort by "mtime" which is sort time based the attribute "mtime". This then does not allow to have one file list with two time based sort columns. @PVince81 Can we split this sorting from "data-sort" into "data-sort-type" (by time) and "data-sort-attribute" (expiration time, share time, ...)? |
|
@MorrisJobke feel free |
|
One high-level issue which is bugging me here is that the left navigation is just a filter. But in this case we show different columns, instead of modified date and size. Do we really want to do that? |
|
The attribute |
|
Looks good for now 👍 Something for later: re-render the file list row whenever share info changed, because the expiration should update there too. This is a bit more complicated and might influence/break/obsolete the part that updates the share icon. |
|
@MorrisJobke can you fix this #19513 (comment) ? Then this will need a second reviewer @rullzer @schiesbn |
|
@MorrisJobke can you fix #19513 (comment) and rebase? |
|
Conflicts with master => Moving back to 2 |
MorrisJobke
force-pushed
the
add-expiration-date-row
branch
from
6dd862f to
81efcd4
Compare
Both done. It still works here :) |
|
@MorrisJobke see #19513 (comment) ;) |
|
@rullzer Mind to help me with the unit tests? 🐱 |
|
We're past feature-freeze, moving this addition to 9.2. Hopefully there will be time to adjust the unit tests. |
|
I just added the unit tests ;) |
|
Works 👍 |
| text = OC.Util.relativeModifiedDate(expirationTimestamp); | ||
| } else { | ||
| formatted = t('files_sharing', 'No expiration date set'); | ||
| text = t('files_sharing', 'Never'); |
There was a problem hiding this comment.
@MorrisJobke make it empty as per @jancborchardt's suggestion above ?
There was a problem hiding this comment.
good point, this also makes it easier to spot the real existing expire dates
|
Nice 👍 |
|
@PVince81 @schiessle Your comments are addressed :) |
DeepDiver1975
force-pushed
the
add-expiration-date-row
branch
from
b4cece4 to
fa40ba7
Compare
|
Looks good 👍 JSUnit fail unrelated (fixed separately on master) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Talked about this with @felixboehm
Maybe @PVince81 can have a look at the actual implementation.