Skip to content
This repository has been archived by the owner on Mar 7, 2023. It is now read-only.

adding logo to title #149

Merged
merged 4 commits into from
Mar 13, 2019
Merged

adding logo to title #149

merged 4 commits into from
Mar 13, 2019

Conversation

obadrawi
Copy link
Contributor

adding a logo beside the title Issue #147

@obadrawi obadrawi requested a review from a team as a code owner March 13, 2019 12:56
@obadrawi
Copy link
Contributor Author

/retest

var img_path = path.resolve(__dirname, 'views/static/logo.svg')
if (varkesConfig.logo) {
img_path = varkesConfig.logo;
var tmp = img_path.split(".")
Copy link
Contributor

Choose a reason for hiding this comment

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

The splitting by dot is a bit naive.. what if the file name contains a dot..
That all guessing of content type dependent on image is maybe a bit too much Either we use a library or maybe let's fix the supported types to "svg" only. Then we have no problem with image scaling and we don't have to do magic.

So if a mock wants a custom logo, then it must be provided in svg. Letzt have just a config validation to assure it and we are done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I could limit it to svg only . but the dot will work as long as it's an image file because I am taking the content after the last dot

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm.. that is an argument :)
However, am afraid that other formats will always end up in bad rendering as we do not specify size and are not scaling..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have allowed only svg and added a validation in the config.js file

Copy link
Contributor

@a-thaler a-thaler left a comment

Choose a reason for hiding this comment

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

see my comments

@@ -56,6 +56,9 @@ function configValidation(configJson) {
}
}
}
if (configJson.logo && !configJson.logo.match(/^.+\.(svg)$/)) {
error_message += "\nlogo image must be in svg format"
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeat the actual value, that helps finding the problem. Print the actual pattern to easily see where the problem is

Suggested change
error_message += "\nlogo image must be in svg format"
error_message += "\nlogo " + configJson.logo + " must be a path to an image in svg format, matching pattern '^.+\.(svg)$'"

Copy link
Contributor

@a-thaler a-thaler left a comment

Choose a reason for hiding this comment

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

Please see my one minor comment

@obadrawi obadrawi merged commit 7f2df13 into master Mar 13, 2019
@obadrawi obadrawi deleted the logo_branch branch March 13, 2019 17:56
obadrawi added a commit that referenced this pull request Mar 14, 2019
* adding logo to title (#149)

* adding logo to the title (Issue #147 )

* v0.1.0
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants