-
Notifications
You must be signed in to change notification settings - Fork 220
Conversation
defer func() { downloadAndExtractChartTarball = downloadAndExtractChartTarballOrig }() | ||
knownError := errors.New("error on downloadAndExtractChartTarball") | ||
downloadAndExtractChartTarball = func(chart *models.ChartPackage) error { | ||
ChartDataExistsOrig := charthelper.ChartDataExists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new on Go
, so sorry about the question :P. Why do you capitalize the first letter of variables? Is this a convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that it is exported so it can be referenced in other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update processIcon
to generate icons using fit
or fill
methods
height int | ||
} | ||
|
||
var availableFormats = []iconFormat{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current configuration we are generating images that fill the given height and width. For example, for Traefik the output is:
The problem is the logo is cropped, so some parts of the logo are missing. For image backgrounds is okey because we want to fill it as much as possible. But, for cards we want to show the entire logo. I think we can add another attribute to iconFormat
called strategy
to switch between fill
and fit
methods:
// Strategy resizing method type
type StrategyType uint8
const(
FIT StrategyType = iota
FILL
)
type iconFormat struct {
name string
strategy StrategyType
width int
height int
}
var availableFormats = []iconFormat{
{"100x100", FILL, 100, 100},
{"300x300", FILL, 300, 300},
{"100x100fit", FIT, 100, 100},
{"300x300fit", FIT, 300, 300},
}
This parameter allow us to use an fill
or fit
strategy to resize and crop the image. The result for the same logo is:
I know this image doesn't measure 100 x 100px, but fit
method ensures me that the maximum height or width will be 100px. So, I can center it using background
properties in the CSS.
We need to update the processIcon
method to check the strategy:
var processed *image.NRGBA
if format.strategy == FILL {
processed = imaging.Fill(orig, format.width, format.height, imaging.Center, imaging.Lanczos)
} else {
processed = imaging.Fit(orig, format.width, format.height, imaging.Lanczos)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Angelmmiguel it makes a lot of sense, thanks a lot!
To reduce computation penalty I'd like to reduce the number of processed icons to the minimum. What's in your take the formats (width, height and strategy) that we will need?
b956eef
to
b5af31a
Compare
Codecov Report@@ Coverage Diff @@
## master #98 +/- ##
==========================================
+ Coverage 84.92% 85.06% +0.14%
==========================================
Files 11 12 +1
Lines 597 663 +66
==========================================
+ Hits 507 564 +57
- Misses 61 67 +6
- Partials 29 32 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@migmartri Look who's a gopher! ;)
The decomposition of cache-->charthelper
here looks fine. There may be a way to add some helper code in the future to generalize all the manual http.Get()
code littered around, but that's not a showstopper.
Thanks @jackfrancis for the review. Yes, We should definitely iterate and decompose some of that code in different packages with common dependencies for code sharing. |
681319f
to
f35e005
Compare
* Static files middleware * Add README to middleware
f35e005
to
50f0e50
Compare
@Angelmmiguel The icon changes have been merged. As you can see, now the UI does not show any icons since they are using the old Now, as part of the chartVersion API we include the different icon sizes that have been processed: For example, in http://localhost:8080/v1/charts/stable/jenkins you get the following payload. |
/assets
(we could eventually put Nginx to serve this path directly instead)Chart endpoint:
Closes #43