Skip to content

Build detail page fixes #492

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 6 commits into from
Sep 23, 2024
Merged

Build detail page fixes #492

merged 6 commits into from
Sep 23, 2024

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 20, 2024

Changes

New elements in the detail view

image

Updating times and actually showing a duration, and wrapping

Screencast.from.2024-09-20.18-27-29.webm

And viewing on mobile actually works now, it's very broken in production

Screencast.from.2024-09-20.18-31-13.webm

- Add field labels
- Condense commit/type/PR fields
- Fixes #46, make date and length field update as the build progresses
@agjohnson agjohnson requested a review from a team as a code owner September 20, 2024 23:57
@agjohnson agjohnson requested review from a team and humitos September 20, 2024 23:58
Also adds a "Wrap output" checkbox to the detail view.
This fixes all leftover display issues on mobile
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Looks great to me!

Comment on lines +192 to +194
{% blocktrans %}
Due to the age of this build, the output of this build is no longer available.
{% endblocktrans %}
Copy link
Member

Choose a reason for hiding this comment

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

💯 -- I'm happy we are not supporting this anymore 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I always figured we just drop it, I doubt anyone is looking at 8 year old builds.

Comment on lines +452 to +454
// Always update date and length, as these should update as the build progresses
this.date.valueHasMutated();
this.length.valueHasMutated();
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. It's the first time I see .valueHasMutated(). I quickly tried to find the documentation for it but I wasn't able to 🤷🏼

I understand that we tell Knockout here that the value that comes from the API is automatically re-calculated internally in the object, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, official documentation is light on this I believe. What this does is force KO to think the value changed and most importantly, it causes updates to all observables depending on this value changing.

This is needed because date_display is calculated off of date, but if there is no change to date, there would normally be no update to date_display.

@agjohnson agjohnson merged commit 412951a into main Sep 23, 2024
4 checks passed
@agjohnson agjohnson deleted the agj/build-fixes branch September 23, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants