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

Adds host name to the header #447

Merged
merged 1 commit into from
Apr 9, 2017
Merged

Adds host name to the header #447

merged 1 commit into from
Apr 9, 2017

Conversation

thaffenden
Copy link
Contributor

Adds the host url to the header:

screen shot 2016-07-13 at 17 11 22

PR #295 does the same thing slightly differently but failed tests and is over a year old so created my own (with less css changes as displaying it with the existing header items keeps the UI more visually consistent).
The max width and word wrap have been set to prevent long urls ruining the layout of whole header.

@thaffenden
Copy link
Contributor Author

I've just seen an issue with this pull request where running in master with a large number of slaves and a large number of failures ruins the header layout. I'll update the css to address this, so wouldn't merge it until I have added another commit.

@thaffenden
Copy link
Contributor Author

I've set a max width to the reset stats box so the text now wraps with the words on different lines and reduced the padding for the boxes from 20px to 12 px.
Updated screenshot (with some dummy data trying to show some longer strings):
screen shot 2016-07-15 at 10 29 50

Copy link
Member

@justiniso justiniso left a comment

Choose a reason for hiding this comment

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

👍

@heyman
Copy link
Member

heyman commented Nov 8, 2016

Hm, not sure about this since it won't work when setting the hostname through the host attribute of the Locust class.

@thaffenden
Copy link
Contributor Author

@heyman sorry for delay, only just had a chance to look at this again.
I've added an if block to set the ui's host value based on where it's specified in your run.
Let me know if you have any thoughts or concerns about it.

@PayscaleNateW
Copy link
Contributor

PayscaleNateW commented Jan 26, 2017

I included a similar change to my recent pull request, only putting it lower in the header. I too only looked at the runner's host, not the locusts. My concern was what if different locusts had different hosts? I think the host in the ui should only display if the runner itself has a host.

@thaffenden
Copy link
Contributor Author

thaffenden commented Feb 16, 2017

@PayscaleNateW Awesome, your changes were on my todo list of thing's I'd like to improve in locust, so that saves me the work! Do you have a screenshot of your implementation for comparison?

With regards to getting the actual host data I agree if they are different it doesn't make sense to display one of them, but if you have to pick I assume you would want to show the one set by the master. Is that an actual use case that people do with locust?
@heyman @justiniso Any thoughts on this?

@heyman
Copy link
Member

heyman commented Feb 16, 2017

@thaffenden Yeah, I guess the most sensible thing to do is to only show it if it's been set through the command line options, or in the host attribute of a single Locust class (like you've done).

Other than that, I think I would also like to decrease the font-size of the hostname a bit (maybe 14px and normal font-weight), just to make it a little less prominent, and work better with really long hostnames.

@thaffenden
Copy link
Contributor Author

@heyman I've updated the CSS to your recommended values.
Here's a screenshot with some dummy data long strings:
locustheader

@justiniso
Copy link
Member

@thaffenden you'll want to pull in the latest changes from master. There have been some style updates to the header.

@thaffenden
Copy link
Contributor Author

@justiniso rebased from master.
New screenshot:
locustheader2

@thaffenden
Copy link
Contributor Author

Rebased again due to other UI changes that have been merged to master.
New screenshot:
locust_header

@thaffenden
Copy link
Contributor Author

@justiniso @heyman is there anything else you wanted doing to this one or is it good to go?

@justiniso justiniso merged commit 349c491 into locustio:master Apr 9, 2017
@justiniso
Copy link
Member

Thanks @thaffenden !

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.

4 participants