-
Notifications
You must be signed in to change notification settings - Fork 28
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
Local fonts and background local image #295
Conversation
lib/scarpe/wv/background.rb
Outdated
@@ -1,13 +1,18 @@ | |||
# frozen_string_literal: true | |||
|
|||
require_relative "../base64" |
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'd prefer require "scarpe/base64" here since at some point we want to separate the core Shoes API into a different gem than Webview.
lib/scarpe/wv/font.rb
Outdated
@@ -0,0 +1,34 @@ | |||
# frozen_string_literal: true | |||
|
|||
require_relative "../base64" |
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.
And same here.
lib/scarpe/wv/image.rb
Outdated
require "uri" | ||
# require "base64" | ||
# require "uri" | ||
require_relative "../base64" |
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.
Remove rather than commenting out the lines above (plus same require request as above.)
Other than the small changes mentioned above, this is great! And since "font" was the first obstacle to running Hackety-Hack, we should update #166 after this gets merged! |
remove puts update from review
removed them |
Two examples failing (https://github.com/scarpe-team/scarpe/actions/runs/5487079739/jobs/9998032426?pr=295). One is a timeout and probably not your fault - I'll download the logs and have a look. The other is about background_color and is likely to repeat. I'm hitting "restart" on the test run. |
ah not sure why it passing now.. hmm |
hmm why do we even get array here.. |
Also this has some conflicts that want a manual resolve to get to head-of-main. |
ah ok so it basically has to do with when we call background red here we are getting array as return |
related to #249 |
oh hm ha i will update tht along with handlign array case |
lib/scarpe/wv/font.rb
Outdated
h.style do | ||
<<~CSS | ||
@font-face { | ||
font-family: Pacifico; |
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.
Huh. Can we change this so it's not always Pacifico?
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.
ah oh we can its just a custom name but like get name from filepath?
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.
That sounds like a good way to do it, yeah.
I mean, if we can't customise then we can't, but it seems worth a try.
okay will do , for some reason my git kraken not opening lol |
changed |
lib/scarpe/base64.rb
Outdated
directory_path = File.dirname(__FILE__, 3) | ||
|
||
image_path = File.join(directory_path, image_filename) | ||
puts "directory_path: #{directory_path}" |
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.
Should probably remove this puts.
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.
ah hm ha sorry abt tht
remove puts
removed |
Looks good. Once the test finishes I'll merge. |
Description
Image(if needed, helps for a faster review)
Checklist