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

Split date from time and position it before URI-R in listing #566

Merged
merged 10 commits into from
Sep 25, 2018

Conversation

ibnesayeed
Copy link
Member

This PR changes URI listing from:

  • memento.us/ (20161231110001)
  • archive.org/ (20080430204826)

to

  • [2016-12-31 11:00:01] memento.us/
  • [2008-04-30 20:48:26] archive.org/

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #566   +/-   ##
=======================================
  Coverage   23.84%   23.84%           
=======================================
  Files           6        6           
  Lines        1191     1191           
  Branches      179      179           
=======================================
  Hits          284      284           
  Misses        887      887           
  Partials       20       20

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 7558bc7...971249a. Read the comment docs.

Copy link
Member

@machawk1 machawk1 left a comment

Choose a reason for hiding this comment

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

I'm not a fan of the date-first styling, despite the consistent alignment of the URI. Perhaps this would be more convincing when we surface the titles in lieu of the URI-Rs but for now it is (subjectively) inferior to URI-R (date).

@ibnesayeed
Copy link
Member Author

The primary reason of putting dates first was a good readable alignment (as you noted it). Also, the date is not linked, so it should not look as bad because the colored links will capture the gaze first.

@machawk1
Copy link
Member

@ibnesayeed I disagree. Maybe it's the implicit precedence of putting the date before the URI-R, the switch to a longer (but more visually parse-able) date string (missing TZ, btw), or the square brackets over parens but I prefer the method displayed in current master over what this PR is offering despite its (mentioned) merits.

Please add a little more to this PR per above and #178 (title) for approval.

@ibnesayeed
Copy link
Member Author

ibnesayeed commented Sep 19, 2018

Parens were good when the date time was rendered after the URL (and title in the future), but if we want to place it up front, perhaps square brackets would make more sense.

With dates and times split with delimiters, placing it after the link looks really ugly when half of it breaks into a new line.

@ibnesayeed
Copy link
Member Author

Non-wrapped entries

datetime after

ipwb-urilist-nowrap-date-after

datetime before

ipwb-urilist-nowrap-date-before

Wrapped entries

datetime after

ipwb-urilist-date-after

datetime before

ipwb-urilist-date-before

I find the datetime before the URI more readable and well aligned in both the cases. That said, there is the possibility to force the datetime record never wrap, but it didn't look very good (to me) either.

@machawk1
Copy link
Member

I see another merit in date-first in that it will always be displayed in the case of a long URI. We ought to adjust the text-overflow property of the URIs. Let's still get the titles working. Let me sleep on this PR.

@machawk1 machawk1 merged commit 95d9c7b into master Sep 25, 2018
@machawk1 machawk1 deleted the split-datetime branch September 25, 2018 16:18
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.

2 participants