Fix large gap between rows of chart-list

Dean-Coakley commented 5 years ago

Fixes: https://github.com/helm/hub/issues/115

Set a height value on .chart-list to fix unnecessary gap between rows of .chart-items

Angelmmiguel commented 5 years ago

Thanks you both for reporting and checking this issue!

As @absoludity mentioned, the problem is that app-chart-list is trying to fill all the vertical space due to the display: flex; flex: 1; CSS declarations.

However, these declarations seems to be outdated based on the following code:

// https://github.com/helm/monocular/blob/master/frontend/src/app/chart-index/chart-index.component.scss
.chart-list {
  width: 100%;
  max-width: $layout-max-width;
  margin: auto;
  display: flex;
  align-items: center;

  app-chart-list {
    flex: 1;
  }
}

It seems to me that app-chart-list used to be a child node of .chart-list. In the current code, the first child of app-chart-list is always .chart-list, which it's the actual container that contains all the app-chart-item components. This container is the only child of app-chart-list and it's the one in charge of formatting the chart grid using flex.

For that reason, we can remove any reference to flex properties associated to the app-chart-list component:

Thanks!

Dean-Coakley commented 5 years ago

Cool, thanks a lot @Angelmmiguel @absoludity !

I observed that removal of flex: 1; also fixed the issue at the time, but didn't really grasp why. Thanks for the explanation.

prydonius commented 5 years ago

Thanks @Dean-Coakley!

Dean-Coakley commented 5 years ago

@andresmgot This still looks broken https://hub.helm.sh/charts/appscode

Did this PR not resolve the issue, or has helm hub not been redeployed yet?

andresmgot commented 5 years ago

I am not a maintainer of the helm hub so maybe @prydonius or @mattfarina can help you with that.

(btw, it works as expected in https://hub.kubeapps.com/charts/appscode)

Dean-Coakley commented 5 years ago

hah oops I meant to tag @Angelmmiguel

Angelmmiguel commented 5 years ago

Hey @Dean-Coakley,

I'm sorry to say I'm on the same situation of @andresmgot. I'm no longer a maintainer of the Helm Hub project 🙂

Dean-Coakley commented 5 years ago

😢

ok, uh.... @absoludity can you answer my question: if monocular has been redeployed/updated on helm hub since the merge of this PR?

absoludity commented 5 years ago

Hi there @Dean-Coakley . Yep, sure - I can confirm just by looking at the page that your change is not yet present on helm hub. You can see that here:

app-chart-list-flex-1

(As Andres said, you can see it has been applied on hub.kubeapps.com)

I'll try to find out first thing next week who looks after helm hub and get it updated :)

absoludity commented 5 years ago

@mattfarina Hi there. Do you know who is now responsible for updating hub.helm.sh? @Dean-Coakley is keen to see his monocular fix deployed to hub.helm.sh (it's a tiny fix for a largish UI issue :) ). I checked the commit history on monocular and it appears to be mostly kubeapps-related contributors - and the changes are already live on hub.kubeapps.com - just not on hub.helm.sh... You seem to be the main contributor on helm/hub, but I see you're not at MS so guessing it's someone else? Thanks for any info.

mattfarina commented 5 years ago

Sorry I've been a little slow responding to this. Doing an update right now isn't a quick change even though it's a chart patch version change.

This change included an update to a dependency on the mongodb chart. Mongodb went through a license change and when applying this update we need to make sure we run a version under the old AGPLv3 license.

In addition to that, there were changes to the chart to move to apps/v1.

We need to do a bit of testing and verification.

The person who normally does the update is me. I'm trying to find the time to work out the details on this more complicated than usual update.

helm / monocular

Fix large gap between rows of chart-list #642