Skip to content
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

Fixes #1861 - Triage Dashboard Design #1935

Merged
merged 12 commits into from
Nov 30, 2017
Merged

Fixes #1861 - Triage Dashboard Design #1935

merged 12 commits into from
Nov 30, 2017

Conversation

magsout
Copy link
Member

@magsout magsout commented Nov 23, 2017

@karlcow

Let's try with this PR ;)

and maybe @miketaylr for the small JS part?

@magsout magsout requested a review from karlcow November 23, 2017 17:39
@magsout magsout changed the title Issues/1861 Fixes #1861 - Triage Dashboard Design Nov 23, 2017
@magsout
Copy link
Member Author

magsout commented Nov 23, 2017

@@ -507,7 +507,7 @@ def add_csp(response):
"font-src 'self'; " +
"img-src 'self' https://www.google-analytics.com https://*.githubusercontent.com data:; " + # nopep8
"manifest-src 'self'; " +
"script-src 'self' https://www.google-analytics.com https://api.github.com; " + # nopep8
"script-src 'self' 'unsafe-inline' https://www.google-analytics.com https://api.github.com; " + # nopep8 @todo removed unsafe-inline

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

Thanks a lot @magsout.
Some comments and requests for change.

Also the footer might need a bit of text :)

>
<option value="">All browsers</option>
<option value="firefox">Firefox</option>
<option value="chrome">Chrome</option>

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

shouldRenderNeedsTriageList()
}

/* readable date*/

This comment was marked as abuse.

}
}

/* handle render or not render NeedsTriageList*/

This comment was marked as abuse.

Triages.classList.add("is-oldest")
break;
default:
alert("tri non supporté")

This comment was marked as abuse.

This comment was marked as abuse.

method="get"
action=""
role="form"
>

This comment was marked as abuse.

</style>

<header class="wc-TopBar" role="banner">
<h1 style="margin: 0; font-size: 1.3em">Triage Dashboard</h1>

This comment was marked as abuse.

if(renderActivityIndicator) {
shouldRenderActivityIndicator()
}
/* liste triage */

This comment was marked as abuse.

This comment was marked as abuse.

}
}

const filteringSort = () => {

This comment was marked as abuse.

This comment was marked as abuse.

@magsout
Copy link
Member Author

magsout commented Nov 24, 2017

Tests and typos are fixed. Just need to figure out how to fix the behaviour of the Sort Filter

@magsout
Copy link
Member Author

magsout commented Nov 24, 2017

@karlcow I fixed sort filtering, but it let one issue about layout with this filter. I Don't know how I can avoid this behavior using flexbox.

Maybe flexbox for this filter is not a good idea, and I have to handle it with JavaScript itself. If you have any idea ;)

edit: Finally, I decided to use order and array.reverse()

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

ok everything looks good apart of the unsafe script.
Let's see what @miketaylr as he has been the one handling most of the CSP.

@@ -507,7 +507,7 @@ def add_csp(response):
"font-src 'self'; " +
"img-src 'self' https://www.google-analytics.com https://*.githubusercontent.com data:; " + # nopep8
"manifest-src 'self'; " +
"script-src 'self' https://www.google-analytics.com https://api.github.com; " + # nopep8
"script-src 'self' 'unsafe-inline' https://www.google-analytics.com https://api.github.com; " + # nopep8 @todo removed unsafe-inline

This comment was marked as abuse.

</main>
<script type="text/javascript">
/* Added an event onSubmit form */
document.getElementById("js-Filters").addEventListener("submit", (e) => {

This comment was marked as abuse.

@@ -507,7 +507,7 @@ def add_csp(response):
"font-src 'self'; " +
"img-src 'self' https://www.google-analytics.com https://*.githubusercontent.com data:; " + # nopep8
"manifest-src 'self'; " +
"script-src 'self' https://www.google-analytics.com https://api.github.com; " + # nopep8
"script-src 'self' 'unsafe-inline' https://www.google-analytics.com https://api.github.com; " + # nopep8 @todo removed unsafe-inline

This comment was marked as abuse.

@miketaylr
Copy link
Member

JS looks fine, just have the question why we can't stick it in an external file, rather than special case the CSP policy.

@magsout
Copy link
Member Author

magsout commented Nov 28, 2017

@miketaylr thanks for the review, I will stick all the js in an external file.

For fat arrow, the compatilibity seems ok right now, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#Browser_compatibility but if you want I can replace by simple functions

 - prettify file
 - remove unsafe-inline scrip src in csp policy
@magsout
Copy link
Member Author

magsout commented Nov 28, 2017

@karlcow @miketaylr done

@magsout
Copy link
Member Author

magsout commented Nov 28, 2017

@karlcow @miketaylr I guess I let css in html files to avoid conflict with the refactor, what do you think ?

@miketaylr
Copy link
Member

Awesome -- thanks! Yeah, CSS should be fine. r+ from me. I'll let @karlcow merge in case he has any final thoughts.

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

@magsout A little English grammar mistake.

and maybe slightly rebasing the commits into something cleaner.

And then we will merge.

Thanks a lot.

href="/issues/{{ issue['number'] }}"
title="{{ issue['title'] }}"
>
More informations

This comment was marked as abuse.

@miketaylr
Copy link
Member

I might take over the history and tweak for @magsout -- I know he's taking care of sick family right now. And I'd like to deploy this today/tomorrow. Thanks!

@miketaylr
Copy link
Member

I'm just gonna squash it all, the commits touch quite a bit of unrelated things.

@miketaylr miketaylr merged commit 9f6e502 into dashboard Nov 30, 2017
@miketaylr miketaylr deleted the issues/1861 branch November 30, 2017 22:03
miketaylr pushed a commit that referenced this pull request Nov 30, 2017
* Issue #1861 - Improvements and filtering for dashboard design
miketaylr pushed a commit that referenced this pull request Nov 30, 2017
* Issue #1861 - Improvements and filtering for dashboard design
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants