Remove inline styles from elements with id='serverstatusquerieschart_… #12823

Merged
merged 7 commits into from Jan 18, 2017

Projects

None yet

3 participants

@clancelotti
Contributor
clancelotti commented Dec 17, 2016 edited

…data'(partial fix of issue #12262)

Signed-off-by: Cristiano Lancelotti lancelot.info@gmail.com

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
@codecov-io
codecov-io commented Dec 17, 2016 edited

Current coverage is 54.21% (diff: 100%)

No coverage report found for master at 0a8d384.

Powered by Codecov. Last update 0a8d384...f683adf

@clancelotti
Contributor
clancelotti commented Dec 17, 2016 edited

Please, help me. It's my first Pull Request. I see I made mistakes using tab for identation instead of spaces although I've read the developer guidelines. What should I do to fix it? Whatever suggestion is welcome...

@clancelotti @clancelotti clancelotti Remove inline styles from elements with id='serverstatusquerieschart_…
…data'(partial fix of issue #12262)

Signed-off-by: Cristiano Lancelotti <lancelot.info@gmail.com>
d3899dd
@clancelotti
Contributor

I changed the files and push again without tab for identation. I used git amend command and push with --force option. If there is something else that I can do, say me.

libraries/server_status_queries.lib.php
@@ -135,7 +135,7 @@ function PMA_getHtmlForServerStatusQueriesDetails($ServerStatusData)
$retval .= '</table>';
$retval .= '<div id="serverstatusquerieschart"></div>';
- $retval .= '<div id="serverstatusquerieschart_data" style="display:none;">';
+ $retval .= '<div id="serverstatusquerieschart_data">';
@nijel
nijel Dec 20, 2016 Member

IMHO in this particular case the div should be rather removed and the data moved to data attribute of serverstatusquerieschart div.

@nijel nijel self-assigned this Dec 20, 2016
@clancelotti clancelotti Remove unnecessary div and move data attibute to serverstatusqueriesc…
…hart

Signed-off-by: Critiano Lancelotti <lancelot.info@gmail.com>
3330e4a
@clancelotti
Contributor
clancelotti commented Dec 22, 2016 edited

nijel, thank you for the tip. I made the changes, but probably I made some mistakes. One check was not successful.

@nijel
Member
nijel commented Dec 22, 2016

Yes, the test needs to be adjusted to new output...

@clancelotti clancelotti Adjust the test to new output
Signed-off-by: Critiano Lancelotti <lancelot.info@gmail.com>
059a43b
@nijel
Member
nijel commented Jan 2, 2017

Have you tested that charts still work? It does not work for me (with some JSON parsing errors).

@clancelotti
Contributor

@nijel, I tested after seeing your message and it does not work for me too. I will try to fix it. If you have some suggestion, please, say me.

@nijel
Member
nijel commented Jan 5, 2017

My original intention was to store the value in the data attribute instead of element content. In this case I think it's some content being added to the element and it tries to parse it instead.

clancelotti added some commits Jan 8, 2017
@clancelotti clancelotti Fix the problem of chart is not rendered
Signed-off-by: Critiano Lancelotti <lancelot.info@gmail.com>
0972666
@clancelotti clancelotti Merge branch 'master' into issue_12262
d3d973f
@clancelotti
Contributor

I fixed the problem and it works for me (commit 0972666...), chart is normally rendered. But there was one conflict after push command, I resolved it and merge on GitHub. Then it generated another commit and the tests occurred on it (commit d3d973f...). I don't know if it is right. Take a look, please.

js/server_status_queries.js
@@ -16,9 +16,10 @@ AJAX.registerOnload('server_status_queries.js', function () {
// Build query statistics chart
var cdata = [];
try {
- $.each(JSON.parse($('#serverstatusquerieschart_data').text()), function (key, value) {
+ $.each(jQuery.parseJSON($('#serverstatusquerieschart').text()), function (key, value) {
@nijel
nijel Jan 8, 2017 Member

Is this change really necessary? jQuery.parseJSON is deprecated....

@clancelotti
clancelotti Jan 8, 2017 Contributor

I tested and it is not necessary. I'm going to fix it now.

clancelotti added some commits Jan 8, 2017
@clancelotti clancelotti Exchange jQuery.parseJSON() for JSON.parse() 2c11b65
@clancelotti clancelotti Merge branch 'issue_12262' of https://github.com/clancelotti/phpmyadmin
… into issue_12262
f683adf
@clancelotti
Contributor
clancelotti commented Jan 17, 2017 edited

@nijel, when I have tested the exhibition of chart I saw that its legend is not properly placed with 2 columns. The same problem occurs on demo server. Do you think it's a good idea to open a PR to fix it?

@nijel
Member
nijel commented Jan 18, 2017

Yes, feel free to go ahead.

@nijel nijel merged commit f683adf into phpmyadmin:master Jan 18, 2017

4 checks passed

Scrutinizer No new issues
Details
codecov/patch 100% of diff hit (target 54.21%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +45.78% compared to b40e9c1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nijel
Member
nijel commented Jan 18, 2017

Sorry, but your code still did not work for me. I've changed it to use data attribute in 9503e0f and now it works fine.

@nijel nijel added this to the 4.7.0 milestone Jan 18, 2017
@clancelotti
Contributor
clancelotti commented Jan 20, 2017 edited

Anyway, thank you very much for deploying the correct solution for this problem instead of just closing the PR. I'm learning with this example. I go ahead and try again.

@clancelotti clancelotti deleted the clancelotti:issue_12262 branch Jan 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment