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

update last released timestamp to visually connect it more to the latest version box #3481

Merged
merged 4 commits into from
Apr 28, 2018

Conversation

cheungnj
Copy link
Contributor

@cheungnj cheungnj commented Mar 30, 2018

This is a WIP to get quicker feedback regarding #3335

This is what I have so far
screen shot 2018-03-30 at 12 13 57 am

Fixes #3335.

@@ -177,3 +177,8 @@ def contains_valid_uris(items):

def parse_version(version_str):
return packaging.version.parse(version_str)


def uncapitalize(text):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jinja has the built-in capitalize filter so I could also:

  1. remove this filter
  2. update convertToReadableText to return all lowercase values
  3. then invoke the capitalize filter on any existing references to humanize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could also use the lower filter. There's a lot of options...

Copy link
Contributor

Choose a reason for hiding this comment

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

lower would be the easiest approach, yet it reads wrong when displaying a date, i.e Released apr 19 2018 vs Released Apr 19 2018 (ideally Released on April 19 2018).

I think we should separate the Jinja side from the Javascript side. If you're up for it we could:

  • Extend the function timeago.js to read an optional suffix from a second attribute and deal with the capitalization if present.
  • Remove Released text and the uncapitalized filter so it simply displays the date as before.

Copy link
Contributor Author

@cheungnj cheungnj Apr 1, 2018

Choose a reason for hiding this comment

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

Definitely up for it but if I'm reverting my changes so the date is displayed as before, then do we really need to extend timeago.js? It seems like the optional parameter would never get used unless I'm misunderstanding something because it seems like we're still going to display the date as Apr 19 2018 regardless.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think prefixing the Released would be a nice touch, but it might not justify the change? thoughts @brainwane @nlhkabu?

@@ -91,7 +91,12 @@ <h1 class="package-header__name">
{% else %}
<a class="status-badge status-badge--good" href="{{ request.route_path('packaging.project', name=release.project.name) }}">Latest Version</a>
{% endif %}
<p class="package-header__date">{{ humanize(release.created) }}</p>
<p class="package-header__date">
<span class="status-badge">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if this was acceptable use of the status-badge class but I could also try using the badge class.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the appropriate class

@brainwane
Copy link
Contributor

@yeraydiazdiaz if you have any time to give your thoughts here, I'd appreciate it.

Copy link
Contributor

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

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

Hi @cheungnj and thanks for your PR, looks much nicer. Left some comments for you, one in particular for changes in Javascript, let me know if you need any help.

@@ -91,7 +91,12 @@ <h1 class="package-header__name">
{% else %}
<a class="status-badge status-badge--good" href="{{ request.route_path('packaging.project', name=release.project.name) }}">Latest Version</a>
{% endif %}
<p class="package-header__date">{{ humanize(release.created) }}</p>
<p class="package-header__date">
<span class="status-badge">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the appropriate class

@@ -177,3 +177,8 @@ def contains_valid_uris(items):

def parse_version(version_str):
return packaging.version.parse(version_str)


def uncapitalize(text):
Copy link
Contributor

Choose a reason for hiding this comment

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

lower would be the easiest approach, yet it reads wrong when displaying a date, i.e Released apr 19 2018 vs Released Apr 19 2018 (ideally Released on April 19 2018).

I think we should separate the Jinja side from the Javascript side. If you're up for it we could:

  • Extend the function timeago.js to read an optional suffix from a second attribute and deal with the capitalization if present.
  • Remove Released text and the uncapitalized filter so it simply displays the date as before.

margin-top: 10px;

&:before {
border-color: rgba(0, 0, 0, 0.4);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe best to use the border color defined in colours.scss

@cheungnj
Copy link
Contributor Author

cheungnj commented Apr 1, 2018

This is what I have now. I addressed all of the comments besides extending the functionality of timeago.js because I wanted some clarification.
screen shot 2018-04-01 at 12 47 16 pm

<p class="package-header__date">{{ humanize(release.created) }}</p>
<p class="package-header__date">
<span class="status-badge">
{{ humanize(release.created) }}
Copy link
Member

Choose a reason for hiding this comment

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

I think prefixing this with a Released: would be appropriate (not sure I totally follow the discussion in this PR about this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe @yeraydiazdiaz meant prefix instead of suffix and the presence of that would cause the human readable date to be lowercase?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how adding a prefix would make the date lowercase? To be clear, I'm talking about changing this line to something like:

Released on: {{ humanize(release.created) }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get that but doesn't Released on: Just now or Released on: About 2 hours ago sound kinda weird? I think the discussion earlier was talking about passing the optional prefix and if it was a truthy value then we'd make the return value of convertToReadableText lowercase. In that case we'd need to pass the prefix only when we're converting the value for the time element in .package-header__date. Maybe just doing Released: is the way to go.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'll leave it to you to decide what sounds best, but I think the simplest approach possible here is probably ideal. Last released: might also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a winner to me

@nlhkabu nlhkabu self-requested a review April 3, 2018 06:31
@cheungnj
Copy link
Contributor Author

cheungnj commented Apr 6, 2018

I forgot to add a new image so this is what it looks like right now:
screen shot 2018-04-06 at 2 21 05 pm

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 17, 2018

hi @cheungnj thanks for this PR. Just to let you know that this is on my list of things to pull down and review this week, so I'll have feedback for you soon :) Thanks for your patience.

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 23, 2018

hi @cheungnj thanks again for this PR :)

Right now, we have some feedback asking us to reduce the whitespace in our UIs (see #1988), so I think my preference here is to keep this style simple (and taking up less space), and just add the 'Last updated' text for now.

It is likely that we will redesign this header altogether based on user feedback, so I will keep this design in mind for then.

@cheungnj cheungnj force-pushed the add-last-released-to-timestamp branch from 9921488 to ee756d6 Compare April 28, 2018 20:58
@cheungnj
Copy link
Contributor Author

Here's what I have now:
screen shot 2018-04-28 at 1 58 36 pm

@di di dismissed yeraydiazdiaz’s stale review April 28, 2018 21:02

addressed/out of date

@di di merged commit cc0ab5d into pypi:master Apr 28, 2018
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.

None yet

5 participants