Skip to content

Crate details page: show whether a build failed in the Versions list #546

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

Merged
merged 3 commits into from
Jan 8, 2020
Merged

Crate details page: show whether a build failed in the Versions list #546

merged 3 commits into from
Jan 8, 2020

Conversation

yvrhdn
Copy link

@yvrhdn yvrhdn commented Jan 4, 2020

Implements second change related to #516.
I was planning to rebase these commits on top of the commits in #543 if that PR gets approved & merged. But this can be merged indepedently.

This pull request changes the Crate Details page to display whether a build failed in the Versions list.

Current design:

Screenshot 2020-01-04 at 18 59 21

Page after these commits:

Screenshot 2020-01-04 at 18 59 32

I performed the changes in two commits, this should make it a bit easier to reason about them. Hopefully...

521b8ae

da358f8

  • In CrateDetails I replaced the field versions with releases. The type changed from a raw String to Release, which holds a version string and a build_status boolean.
  • I create a fn versions() which has the same behaviour as the removed versions field.
  • I set releases in CrateDetails::new but I took care to change as little code as possible. Hence the introduction of map_to_release. This entire function can be refactored to be more efficient / perform less queries on the database, but that's for a later PR.
  • Lastly, I edited crate_details.hbs to use the additional information and show a ⚠️ sign when appropriate.

@yvrhdn yvrhdn changed the title Show failed builds Crate details page: show whether a build failed in the Versions list Jan 4, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

In general this looks good! You'll have to rebase over master since #543 got merged. I also left a few minor nits.

@yvrhdn
Copy link
Author

yvrhdn commented Jan 7, 2020

I have rebased to the latest commits on master.

Nitpicks have been resolved 👍

And I have simplified the versions display by using a tooltip:

Screenshot 2020-01-07 at 22 58 53

@yvrhdn yvrhdn requested a review from jyn514 January 7, 2020 22:00
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Looks good to merge once you fix the nit :)

@jyn514
Copy link
Member

jyn514 commented Jan 8, 2020

I'm really looking forward to having this, thanks again for your hard work!

Koenraad Verheyden added 2 commits January 8, 2020 02:31
CrateDetails: replace field 'versions' with 'releases', which contains both the
version and the build status of a release. Since CrateDetails::versions was a
public field, CrateDetails::versions() is designed to be compatible with the
previous exposed field.
@jyn514 jyn514 merged commit 8975271 into rust-lang:master Jan 8, 2020
@yvrhdn yvrhdn deleted the show_failed_builds_#516 branch January 8, 2020 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