Repository navigation
Fixes #1129 - #1130
Fixes #1129#1130dmelikhov wants to merge 1 commit into
Conversation
|
Hey @Dibro89, thanks for doing this PR! Have you tried to run it locally? I'm finding some issues:
I still hadn't the time to look through the code, will try to do it ASAP, but it seems we have some tweaks to do to make it work as before at least |
| @download = @download64 | ||
| @bitness = "64-bit" | ||
| else | ||
| @download = @download32 |
There was a problem hiding this comment.
these download and bitness variables are passed to the template. Since they are not defined now, we see the (#{@download32.version.name}) in the rendered page.
There was a problem hiding this comment.
We are not using download and bitness ruby variables in the template anymore.
You are seeing this because I don't know how to properly inject ruby variables into JS-code blocks. :)
There was a problem hiding this comment.
haven't dig enough to be sure about this, but don't we need the download variable to start the download variable automatically? (I'll find sometime today to have a deeper look on this)
There was a problem hiding this comment.
@pedrorijo91 Before you were detecting 'bitness' and the correct version to download on the server side, but I've changed it in a way so everything now depends on the client side. You can take a look at another file in the commit and see how it is should work now. Download link is also detected on the client side and then placed in an iframe as before.
|
@pedrorijo91 Sorry, I'm not really good with Ruby. |
|
no worries @Dibro89 :) I'm not sure how to inject js variables too (not sure if possible at all). But you can simply add a |
|
@pedrorijo91 The text is already injecting from JS into HTML, but we also need to inject it into JS. |
|
@pedrorijo91 Please, try after this commit. |
|
Did you updated the PR @Dibro89 ? maybe you forgot to push? |
|
@pedrorijo91 I did, but I've also squashed some commits, I thought you will be able to pull. |
|
If you don't push your commits I can't pull them @Dibro89, or am I missing something? As you can see, this PR only contains the commit you did yesterday |
|
@pedrorijo91 I've pushed a second commit after it and then squashed them with the name of first. You can look at the date - 7 hours from now. Just re-clone the repo please. |
|
there's something weird going on...where are you seeing 7 hours from now? I see 1 day ago
|
|
In history I have the same, true. But in the conversation I can see this: |
pedrorijo91
left a comment
pedrorijo91
left a comment
There was a problem hiding this comment.
It seems to be working just fine :) thanks for the effort! just some comments, let me know your thoughts
| var download_url = '<%= @download32.url %>'; | ||
| } | ||
|
|
||
| document.write( |
There was a problem hiding this comment.
I'm not an expert on js, but from what I know, document.write is not recommended, and other approaches such DOM manipulation are preferred.
https://stackoverflow.com/questions/4537963/what-are-alternatives-to-document-write
If you have a good argument for using document.write (if you have a nice experience in JS and are sure about this), let us know, and I won't oppose :)
There was a problem hiding this comment.
I'm definitely not an expert too. :)
But I suppose if we use DOM-manipulation - it can create a slight (maybe even not noticeable) delay.
And I don't see any reason why we should not use document.write(...) there.
There was a problem hiding this comment.
i don't think the delay would be noticeable since it's just a small piece of logic. And having placeholders on the DOM + moving this code into a .js file, seems a cleaner approach than having these <script> tags in the middle of the HTML.
since the issue is already fixed by the new cache rule (#1129 (comment)), I would prefer to have the clean approach.
There was a problem hiding this comment.
Hey @Dibro89, are you interested in trying the other approach? Otherwise I may try it by myself so that we can compare :)
| <p> | ||
| <%= raw "<strong>If your download hasn't started, <a href=\"#{@download.url}\">click here to download manually</a>.</strong>" %> | ||
| </p> | ||
| <script> |
There was a problem hiding this comment.
can we move this script into some .js file? there's a app/assets/javascripts/site.js with most of the javascript used in this app
There was a problem hiding this comment.
I think we can't because it is injecting HTML just in time and it is not possible if we move the script somewhere else.
But if you want to manipulate DOM – we can move it.
| <%= raw "The current source code release is version <strong>#{@latest.name}</strong>. If you want the newer version, you can build it from <a href=\"#{@source_url}\">the source code</a>." %> | ||
| </p> | ||
| <iframe frameborder="0" height="1" src="<%= @download.url %>" width="1"></iframe> | ||
| <script> |
|
the issue is solved, will postpone this change for #1143 |
No description provided.