[review-request] adamian/20150507_toasterui_fixes

[review-request] adamian/20150507_toasterui_fixes

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

Hello,

This a refactoring of the URL structure to be more REST-ful.
It remove the usage of the server-side user session to remember currently
selected project, making navigation more robust, opening API for usage to
other tools, and passing the crawl test.

Cheers,
Alex

-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 863 bytes --]

Re: [review-request] adamian/20150507_toasterui_fixes

[Michael Wood]

Spotted one error I think

The project page link  r'^project/(?P<pid>\d+)$' should be 
r'^project/(?P<pid>\d+)/$

i.e.

-        url(r'^project/(?P<pid>\d+)$', 'project', name='project'),
+        url(r'^project/(?P<pid>\d+)/$', 'project', name='project'),

Your django might not have reloaded the urls so didn't catch it maybe?

recreate:

1. go to /projects/
2. Select a project - 404

Michael

On 08/05/15 17:39, Michael Wood wrote:
> Looks fine to me.
>
> Michael
>
> On 07/05/15 18:13, Damian, Alexandru wrote:
>> Hello,
>>
>> This a refactoring of the URL structure to be more REST-ful.
>> It remove the usage of the server-side user session to remember 
>> currently selected project, making navigation more robust, opening 
>> API for usage to other tools, and passing the crawl test.
>>
>> Cheers,
>> Alex
>>
>> -- 
>> Alex Damian
>> Yocto Project
>> SSG / OTC
>>
>>
>

Re: [review-request] adamian/20150507_toasterui_fixes

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

Umm, this particular issue doesn't happen to me.

I've pushed up a new version with more fixes.

On Fri, May 8, 2015 at 5:53 PM, Michael Wood <michael.g.wood@intel.com>
wrote:

> Spotted one error I think
>
> The project page link  r'^project/(?P<pid>\d+)$' should be
> r'^project/(?P<pid>\d+)/$
>
> i.e.
>
> -        url(r'^project/(?P<pid>\d+)$', 'project', name='project'),
> +        url(r'^project/(?P<pid>\d+)/$', 'project', name='project'),
>
> Your django might not have reloaded the urls so didn't catch it maybe?
>
> recreate:
>
> 1. go to /projects/
> 2. Select a project - 404
>
> Michael
>
>
> On 08/05/15 17:39, Michael Wood wrote:
>
>> Looks fine to me.
>>
>> Michael
>>
>> On 07/05/15 18:13, Damian, Alexandru wrote:
>>
>>> Hello,
>>>
>>> This a refactoring of the URL structure to be more REST-ful.
>>> It remove the usage of the server-side user session to remember
>>> currently selected project, making navigation more robust, opening API for
>>> usage to other tools, and passing the crawl test.
>>>
>>> Cheers,
>>> Alex
>>>
>>> --
>>> Alex Damian
>>> Yocto Project
>>> SSG / OTC
>>>
>>>
>>>
>>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>


-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 2840 bytes --]

Re: [review-request] adamian/20150507_toasterui_fixes

[Michael Wood]

Looks good to me.

Thanks,

Michael

On 13/05/15 14:10, Damian, Alexandru wrote:
> I found a solution using <style> and scoped.
>
> Can you please review on the same branch ?
>
> Cheers,
> Alex
>
> On Wed, May 13, 2015 at 10:42 AM, Damian, Alexandru 
> <alexandru.damian@intel.com <mailto:alexandru.damian@intel.com>> wrote:
>
>
>
>     On Tue, May 12, 2015 at 7:06 PM, Michael Wood
>     <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>> wrote:
>
>
>         Thanks, this fixes a bunch of the url issues for me, I also
>         had to delete the urls.pyc as well as I was getting some
>         strange issues (like where the project links weren't working)
>
>         Only one issue, what was the reason for doing this?:
>
>
>         --- a/bitbake/lib/toaster/toastergui/templates/layerdetails.html
>         +++ b/bitbake/lib/toaster/toastergui/templates/layerdetails.html
>         @@ -37,27 +37,18 @@
>                document.write("Sorry, An error has occurred loading
>         this page");
>                console.warn(e);
>              }
>         +
>         +    {# If this is not an imported layer then hide the edit ui #}
>         +    {% if layerversion.layer_source.sourcetype !=
>         layerversion.layer_source.TYPE_IMPORTED %}
>         +    $(".icon-pencil").hide();
>         +    $(".delete-current-value").hide();
>         +    $(".icon-trash").hide();
>         +    $(".add-deps").hide();
>         +    {% endif %}
>         +
>            });
>          </script>
>
>         -{# If this is not an imported layer then hide the edit ui #}
>         -{% if layerversion.layer_source_id != 3 %}
>         -<style>
>         - .icon-pencil {
>         -   display:none;
>         - }
>         -.delete-current-value{
>         -  display: none;
>         -}
>         - li .icon-trash {
>         -   display:none;
>         - }
>         - .add-deps {
>         -   display:none;
>         - }
>         -</style>
>         -{% endif %}
>         -
>
>         The advantage of doing it in the style/document is that you
>         don't get a visible state change in the page when the page is
>         loaded.
>         e.g. doing this is in the JS will do: All visible ->  Hide the
>         .icon-pencil, Hide the .delete-current-value etc all in front
>         of the user.
>
>
>     ​ Unfortunately, the <style> tag can only occur ​in certain places
>     under HTML5 specification; the validator complains about its
>     presence there. I think the presence there leads to undefined
>     behaviour in the specification, and it's not guaranteed to work in
>     future browsers.
>
>
>         I'd rather have the page state setup correctly at render when
>         possible, if you really want to do this in the JS it needs to
>         be inside the
>         ​ ​
>         layerDetailsPageInit function and everything hidden by default
>         then shown once the hide/shows inside the page are done.
>
>
>     Yep, this would be a better solution. I'm going to push a patch on
>     top of this one to reverse the behaviour.
>
>     ​
>
>
>         Thanks,
>
>         Michael
>
>
>         On 12/05/15 18:18, Damian, Alexandru wrote:
>
>             Hello,
>
>             This is a review request for a single patch that brings
>             all views in compliance with W3C's HTML5 markup validation
>             service.
>
>             It also fixes minor problems discovered during validation.
>
>             Can you please review ?
>
>             Alex
>
>             -- 
>             Alex Damian
>             Yocto Project
>             SSG / OTC
>
>
>
>         ---------------------------------------------------------------------
>         Intel Corporation (UK) Limited
>         Registered No. 1134945 (England)
>         Registered Office: Pipers Way, Swindon SN3 1RJ
>         VAT No: 860 2173 47
>
>         This e-mail and any attachments may contain confidential
>         material for
>         the sole use of the intended recipient(s). Any review or
>         distribution
>         by others is strictly prohibited. If you are not the intended
>         recipient, please contact the sender and delete all copies.
>
>
>
>
>     -- 
>     Alex Damian
>     Yocto Project
>     SSG / OTC
>
>
>
>
> -- 
> Alex Damian
> Yocto Project
> SSG / OTC
>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>

Re: [review-request] adamian/20150507_toasterui_fixes

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 3672 bytes --]

I found a solution using <style> and scoped.

Can you please review on the same branch ?

Cheers,
Alex

On Wed, May 13, 2015 at 10:42 AM, Damian, Alexandru <
alexandru.damian@intel.com> wrote:

>
>
> On Tue, May 12, 2015 at 7:06 PM, Michael Wood <michael.g.wood@intel.com>
> wrote:
>
>>
>> Thanks, this fixes a bunch of the url issues for me, I also had to delete
>> the urls.pyc as well as I was getting some strange issues (like where the
>> project links weren't working)
>>
>> Only one issue, what was the reason for doing this?:
>>
>>
>> --- a/bitbake/lib/toaster/toastergui/templates/layerdetails.html
>> +++ b/bitbake/lib/toaster/toastergui/templates/layerdetails.html
>> @@ -37,27 +37,18 @@
>>        document.write("Sorry, An error has occurred loading this page");
>>        console.warn(e);
>>      }
>> +
>> +    {# If this is not an imported layer then hide the edit ui #}
>> +    {% if layerversion.layer_source.sourcetype !=
>> layerversion.layer_source.TYPE_IMPORTED %}
>> +    $(".icon-pencil").hide();
>> +    $(".delete-current-value").hide();
>> +    $(".icon-trash").hide();
>> +    $(".add-deps").hide();
>> +    {% endif %}
>> +
>>    });
>>  </script>
>>
>> -{# If this is not an imported layer then hide the edit ui #}
>> -{% if layerversion.layer_source_id != 3 %}
>> -<style>
>> - .icon-pencil {
>> -   display:none;
>> - }
>> -.delete-current-value{
>> -  display: none;
>> -}
>> - li .icon-trash {
>> -   display:none;
>> - }
>> - .add-deps {
>> -   display:none;
>> - }
>> -</style>
>> -{% endif %}
>> -
>>
>> The advantage of doing it in the style/document is that you don't get a
>> visible state change in the page when the page is loaded.
>> e.g. doing this is in the JS will do: All visible ->  Hide the
>> .icon-pencil, Hide the .delete-current-value etc all in front of the user.
>>
>
> ​Unfortunately, the <style> tag can only occur ​in certain places under
> HTML5 specification; the validator complains about its presence there. I
> think the presence there leads to undefined behaviour in the specification,
> and it's not guaranteed to work in future browsers.
>
>
>>
>> I'd rather have the page state setup correctly at render when possible,
>> if you really want to do this in the JS it needs to be inside the
>> ​​
>> layerDetailsPageInit function and everything hidden by default then shown
>> once the hide/shows inside the page are done.
>>
>
> Yep, this would be a better solution. I'm going to push a patch on top of
> this one to reverse the behaviour.
>
> ​
>
>
>>
>> Thanks,
>>
>> Michael
>>
>>
>> On 12/05/15 18:18, Damian, Alexandru wrote:
>>
>>> Hello,
>>>
>>> This is a review request for a single patch that brings all views in
>>> compliance with W3C's HTML5 markup validation service.
>>>
>>> It also fixes minor problems discovered during validation.
>>>
>>> Can you please review ?
>>>
>>> Alex
>>>
>>> --
>>> Alex Damian
>>> Yocto Project
>>> SSG / OTC
>>>
>>>
>>>
>> ---------------------------------------------------------------------
>> Intel Corporation (UK) Limited
>> Registered No. 1134945 (England)
>> Registered Office: Pipers Way, Swindon SN3 1RJ
>> VAT No: 860 2173 47
>>
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>>
>>
>
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 5942 bytes --]

Re: [review-request] adamian/20150507_toasterui_fixes

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 3278 bytes --]

On Tue, May 12, 2015 at 7:06 PM, Michael Wood <michael.g.wood@intel.com>
wrote:

>
> Thanks, this fixes a bunch of the url issues for me, I also had to delete
> the urls.pyc as well as I was getting some strange issues (like where the
> project links weren't working)
>
> Only one issue, what was the reason for doing this?:
>
>
> --- a/bitbake/lib/toaster/toastergui/templates/layerdetails.html
> +++ b/bitbake/lib/toaster/toastergui/templates/layerdetails.html
> @@ -37,27 +37,18 @@
>        document.write("Sorry, An error has occurred loading this page");
>        console.warn(e);
>      }
> +
> +    {# If this is not an imported layer then hide the edit ui #}
> +    {% if layerversion.layer_source.sourcetype !=
> layerversion.layer_source.TYPE_IMPORTED %}
> +    $(".icon-pencil").hide();
> +    $(".delete-current-value").hide();
> +    $(".icon-trash").hide();
> +    $(".add-deps").hide();
> +    {% endif %}
> +
>    });
>  </script>
>
> -{# If this is not an imported layer then hide the edit ui #}
> -{% if layerversion.layer_source_id != 3 %}
> -<style>
> - .icon-pencil {
> -   display:none;
> - }
> -.delete-current-value{
> -  display: none;
> -}
> - li .icon-trash {
> -   display:none;
> - }
> - .add-deps {
> -   display:none;
> - }
> -</style>
> -{% endif %}
> -
>
> The advantage of doing it in the style/document is that you don't get a
> visible state change in the page when the page is loaded.
> e.g. doing this is in the JS will do: All visible ->  Hide the
> .icon-pencil, Hide the .delete-current-value etc all in front of the user.
>

​Unfortunately, the <style> tag can only occur ​in certain places under
HTML5 specification; the validator complains about its presence there. I
think the presence there leads to undefined behaviour in the specification,
and it's not guaranteed to work in future browsers.


>
> I'd rather have the page state setup correctly at render when possible, if
> you really want to do this in the JS it needs to be inside the
> ​​
> layerDetailsPageInit function and everything hidden by default then shown
> once the hide/shows inside the page are done.
>

Yep, this would be a better solution. I'm going to push a patch on top of
this one to reverse the behaviour.

​


>
> Thanks,
>
> Michael
>
>
> On 12/05/15 18:18, Damian, Alexandru wrote:
>
>> Hello,
>>
>> This is a review request for a single patch that brings all views in
>> compliance with W3C's HTML5 markup validation service.
>>
>> It also fixes minor problems discovered during validation.
>>
>> Can you please review ?
>>
>> Alex
>>
>> --
>> Alex Damian
>> Yocto Project
>> SSG / OTC
>>
>>
>>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>


-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 4770 bytes --]

Re: [review-request] adamian/20150507_toasterui_fixes

[Michael Wood]

Thanks, this fixes a bunch of the url issues for me, I also had to 
delete the urls.pyc as well as I was getting some strange issues (like 
where the project links weren't working)

Only one issue, what was the reason for doing this?:


--- a/bitbake/lib/toaster/toastergui/templates/layerdetails.html
+++ b/bitbake/lib/toaster/toastergui/templates/layerdetails.html
@@ -37,27 +37,18 @@
        document.write("Sorry, An error has occurred loading this page");
        console.warn(e);
      }
+
+    {# If this is not an imported layer then hide the edit ui #}
+    {% if layerversion.layer_source.sourcetype != 
layerversion.layer_source.TYPE_IMPORTED %}
+    $(".icon-pencil").hide();
+    $(".delete-current-value").hide();
+    $(".icon-trash").hide();
+    $(".add-deps").hide();
+    {% endif %}
+
    });
  </script>

-{# If this is not an imported layer then hide the edit ui #}
-{% if layerversion.layer_source_id != 3 %}
-<style>
- .icon-pencil {
-   display:none;
- }
-.delete-current-value{
-  display: none;
-}
- li .icon-trash {
-   display:none;
- }
- .add-deps {
-   display:none;
- }
-</style>
-{% endif %}
-

The advantage of doing it in the style/document is that you don't get a 
visible state change in the page when the page is loaded.
e.g. doing this is in the JS will do: All visible ->  Hide the 
.icon-pencil, Hide the .delete-current-value etc all in front of the user.

I'd rather have the page state setup correctly at render when possible, 
if you really want to do this in the JS it needs to be inside the 
layerDetailsPageInit function and everything hidden by default then 
shown once the hide/shows inside the page are done.

Thanks,

Michael

On 12/05/15 18:18, Damian, Alexandru wrote:
> Hello,
>
> This is a review request for a single patch that brings all views in 
> compliance with W3C's HTML5 markup validation service.
>
> It also fixes minor problems discovered during validation.
>
> Can you please review ?
>
> Alex
>
> -- 
> Alex Damian
> Yocto Project
> SSG / OTC
>
>

[review-request] adamian/20150507_toasterui_fixes

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

Hello,

This is a review request for a single patch that brings all views in
compliance with W3C's HTML5 markup validation service.

It also fixes minor problems discovered during validation.

Can you please review ?

Alex

-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 895 bytes --]