Fix large gap between rows of chart-list

[Dean-Coakley]

Fixes: helm/hub#115

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

[absoludity]

Hi there @Dean-Coakley . Thanks for taking a look at this :) I'm always a little wary of just setting something to height: 0 because it fixes the issue, so was keen to understand the problem. Please see what you think:

Problem: As you mentioned on the issue, this affects other lists too - specifically, it affects any list which has enough charts to wrap a row, but not enough to fill the whole app-chart-list container. (ie. if you choose all or stable, where there are lots of charts, there is no issue because the number of charts fills the available space in the container).

So the question is: why is the single app-chart-list container filling the space available by its parent (div.charts__gallery__content)? AFAICT, the reason is that app-chart-list has flex: 1 applied, which means that it will grow to fit whatever size is available in its parent. So the children app-chart-item elements are layed out to fill this space, the height being divided up by the number of rows. So if there is only two rows, the elements of each row have a massive height.

Therefore, I think a better solution would be to remove the flex: 1 on the app-chart-list so that it only uses as much height as required, and therefore when the child rows are spaced out to fit the full height of the parent app-chart-list container, that parent only has the required height for the given rows, rather than growing to the full possible height of it's parent (div.charts__gallery__content) :

I've verified (as above) that disabling the flex: 1 for app-chart-list works on both FF and Chromium. Please try it out and see what you think.

(hmm, I see the app-chart-list element is set to flex: 1 in two places in the css, so I assume both will need removing).

[Angelmmiguel]

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:

• https://github.com/helm/monocular/blob/master/frontend/src/app/charts/charts.component.scss#L44
• https://github.com/helm/monocular/blob/master/frontend/src/app/chart-index/chart-index.component.scss#L11

Thanks!

[Dean-Coakley]

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.

[absoludity]

Great, thanks Dean.

[prydonius]

Thanks @Dean-Coakley!

[Dean-Coakley]

@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]

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]

hah oops I meant to tag @Angelmmiguel

[Angelmmiguel]

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]

😢

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

[absoludity]

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:

(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]

@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]

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.