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

[SPARK-20942][WEB-UI]The title style about field is error in the history server web ui. #18170

Closed
wants to merge 38 commits into from
Closed

Conversation

guoxiaolongzte
Copy link

@guoxiaolongzte guoxiaolongzte commented Jun 1, 2017

What changes were proposed in this pull request?

1.The title style about field is error.
fix before:
before

fix after:
fix

fix1

executor-page style:
executor_page

2.Title text description, 'the application' should be changed to 'this application'.

3.Analysis of code:
$('#history-summary [data-toggle="tooltip"]').tooltip();
The id of 'history-summary' is not there. We only contain id of 'history-summary-table'.

How was this patch tested?

manual tests

Please review http://spark.apache.org/contributing.html before opening a pull request.

郭小龙 10207633 added 30 commits March 31, 2017 21:57
@guoxiaolongzte
Copy link
Author

guoxiaolongzte commented Jun 1, 2017

@srowen @ajbozarth @jerryshao Help to review the code, thanks.

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

Most of this change is unnecessary, everything in historypage-template.html is that way intentionally. Also note my comment in historypage.js

@@ -195,7 +195,7 @@ $(document).ready(function() {
}

$(selector).DataTable(conf);
$('#hisotry-summary [data-toggle="tooltip"]').tooltip();
$('#history-summary-table [data-toggle="tooltip"]').tooltip();
Copy link
Member

Choose a reason for hiding this comment

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

Just #history-summary is still correct here, but nice catch on the spelling error.

Copy link
Author

Choose a reason for hiding this comment

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

The original is the spelling error. I did not notice that I thought it was ID wrong. I will fix it and test it.

Copy link
Author

Choose a reason for hiding this comment

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

I have fixed the spelling error. The test result is OK. But historypage-template.html still need to be modified. Thanks.

App Name
</span>
</th>
<th class="attemptIDSpan">
<span data-toggle="tooltip" data-placement="above" title="The attempt ID of this application since one application might be launched several times">
<span data-toggle="tooltip" data-placement="right" title="The attempt ID of this application since one application might be launched several times">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to change the tooltip placement to "right", any particular issue here?

Copy link
Author

@guoxiaolongzte guoxiaolongzte Jun 1, 2017

Choose a reason for hiding this comment

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

The tooltip placement to "above", the prompt position will always be displayed on the APP ID, rather than on the current title.

1

Copy link
Member

Choose a reason for hiding this comment

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

ah I found the issue, above is incorrect, what we want is top

Copy link
Author

Choose a reason for hiding this comment

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

I will fix it and test it. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

I have fixed the error. ‘top’ replace ‘above’. The test result is OK. Thanks.

App ID
</span>
</th>
<th>
<span data-toggle="tooltip" data-placement="above" title="Name of the application.">
<span data-toggle="tooltip" data-placement="right" title="Name of this application.">
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason to change this

Copy link
Author

@guoxiaolongzte guoxiaolongzte Jun 1, 2017

Choose a reason for hiding this comment

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

Hello, you mean, 'the application' is changed to 'this application'? Or 'above' to 'right'?

First, consistent with other field titles.
11

Second, The tooltip placement to "above", the prompt position will always be displayed on the APP ID, rather than on the current title.

1

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

LGTM, you've addressed all my [valid] concerns

@guoxiaolongzte
Copy link
Author

guoxiaolongzte commented Jun 1, 2017

@ajbozarth
Thanks.This is what I should do.

@SparkQA
Copy link

SparkQA commented Jun 1, 2017

Test build #3770 has finished for PR 18170 at commit 446a584.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@guoxiaolongzte guoxiaolongzte changed the title [SPARK-20942]The title style about field is error in the history server web ui. [SPARK-20942][WEB-UI]The title style about field is error in the history server web ui. Jun 2, 2017
@srowen
Copy link
Member

srowen commented Jun 2, 2017

Merged to master/2.2

asfgit pushed a commit that referenced this pull request Jun 2, 2017
…tory server web ui.

## What changes were proposed in this pull request?

1.The title style about field is error.
fix before:
![before](https://cloud.githubusercontent.com/assets/26266482/26661987/a7bed018-46b3-11e7-8a54-a5152d2df0f4.png)

fix after:
![fix](https://cloud.githubusercontent.com/assets/26266482/26662000/ba6cc814-46b3-11e7-8f33-cfd4cc2c60fe.png)

![fix1](https://cloud.githubusercontent.com/assets/26266482/26662080/3c732e3e-46b4-11e7-8768-20b5a6aeadcb.png)

executor-page style:
![executor_page](https://cloud.githubusercontent.com/assets/26266482/26662384/167cbd10-46b6-11e7-9e07-bf391dbc6e08.png)

2.Title text description, 'the application' should be changed to 'this application'.

3.Analysis of code:
 $('#history-summary [data-toggle="tooltip"]').tooltip();
The id of 'history-summary' is not there. We only contain id of 'history-summary-table'.

## How was this patch tested?

manual tests

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: guoxiaolong <guo.xiaolong1@zte.com.cn>
Author: 郭小龙 10207633 <guo.xiaolong1@zte.com.cn>
Author: guoxiaolongzte <guo.xiaolong1@zte.com.cn>

Closes #18170 from guoxiaolongzte/SPARK-20942.

(cherry picked from commit 625cebf)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@asfgit asfgit closed this in 625cebf Jun 2, 2017
@guoxiaolongzte guoxiaolongzte deleted the SPARK-20942 branch June 12, 2017 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants