Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improve dashboard for usability and bookmarking #54

Closed
tojon opened this Issue May 17, 2013 · 30 comments

Comments

Projects
None yet
3 participants
Contributor

tojon commented May 17, 2013

In starting to use the dashboard, I noticed a couple of things as a new user:

It's not clear what server you are on, unless you peer at the url
eg. http://mozmill-ci.blargon7.com vs. http://mozmill-crowd.blargon7.com

When you bookmark the above two dashboards, they are identical in a bookmark list
eg. "Mozmill Results Dashboard"

So to add clarity when using each server, I propose a small fix which will add readability, and provide for correct bookmarking and help cue the user where they are. I've included a few screen grabs:

The current server appearance:

currentdashboard

Proposed appearance for CI:

proposedcidashboard

Proposed appearance for Crowd:

proposedcrowddashboard

Proposed fallback appearance if the server is changed (same as current appearance):

proposedfallbackdashboard

Note should be made that title.htm in the url is not part of the proposed spec. It was just a stub file I was using for testing.

I've tested this locally with my fork, using a local Apache server with my changes and everything appears to be working fine. I'll provide a link reference to the code so you can see the changes involved. It just adds one line to the index.htm and a little js module which distinguishes the locations.

Contributor

tojon commented May 17, 2013

These are the little changes involved. If you think it's helpful let me know and I could submit a pull request.
tojon/mozmill-dashboard@8b3eba9

Member

davehunt commented May 20, 2013

I like the proposed changes, but we should also consider our other dashboards: mozmill-addons, mozmill-sandbox, mozmill-staging, and mozmill-ondemand. If we could make this dynamic based on the database name that would be ideal.

Contributor

tojon commented May 20, 2013

Sure, I will look into those.

Contributor

whimboo commented May 21, 2013

The best here would be to add a property to the config file for the dashbard name. But that would require use to push different versions of the code to the remote server. To be able to do that we would need different branches.

The solution which has been proposed from Jonathan is hard-coded and would require updates whenever we push anohter db or move the domains.

I would propose that we add a hash to the config file which maps the domain to the appropriate title. That would allow even others to setup their own instances without a hassle.

All the dashboards can be found here:
http://mozauto.iriscouch.com/_utils/index.html

How does that sound?

Contributor

tojon commented May 21, 2013

If the admin neglected to update the config.js hash when creating a new dashboard type or domain, it sounds like it would still gracefully fall back to "Mozmill Results Dashboard" correct? I am assuming that would still be the unaltered title value in the index.html file.

And in implementation, js/title.js would still be used to read the hash map in the config file and manipulate the title, correct?

Contributor

whimboo commented May 22, 2013

I would directly put the code in dashboard.js and not introduce a new file with just a single method. Also a fallback would be nice if the name cannot be determined, yes.

Contributor

tojon commented Jun 16, 2013

Ok, I have something working locally now, and within dashboard.js. The new function will always return "" if there is no server name match; so the page will fallback to "Mozmill Results Dashboard" title values in the index.html. All good.

I noticed however, successfully accessing the title element via its required jQuery html method - $("#title").html() in index.html - required pointing to an alternate jquery.min.js:

(used temporarily during the work)
src="//ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js

(currently in latest revision referenced in index.html and locally in the repo)
src="js/jquery/jquery-1.4.2.min.js

Otherwise the .html method was not found.

Perhaps there is an update of this required first, to support the change? Or accompanying this return?

But otherwise everything appears to be working fine.

Contributor

whimboo commented Jun 17, 2013

I would say let us update jquery first. Our internal copy is kinda old, and might indeed be the problem why it's not working.

Contributor

tojon commented Jul 3, 2013

So we tried an update to jQuery 1.10.1 in mozmill-sandbox but it introduced a problem with the Endurance charts, where the graph lines did not draw. Dave kindly reverted sandbox back to jQuery 1.4.2, and we closed the above Issue and PR.

On further local testing, I now believe I can access the jQuery method with the existing 1.4.2 lib. So I plan to push a new revision of the Title improvement work to my branch for review.

It will contain the centralized controls for the server names and titles in the config.js file as discussed, a fallback to the original document title, and is performance optimized to draw the title as quickly as possible during page load.

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Jul 3, 2013

Contributor

tojon commented Jul 3, 2013

I've pushed the proposed changes to my branch above, for informal review. I will provide a PR after that.

Member

davehunt commented Jul 4, 2013

Looks good, please go ahead and submit a PR

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Jul 4, 2013

Contributor

tojon commented Jul 4, 2013

PR #60

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Jul 24, 2013

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Aug 20, 2013

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Aug 21, 2013

davehunt added a commit to davehunt/mozmill-dashboard that referenced this issue Aug 22, 2013

Member

davehunt commented Aug 22, 2013

Fixed in ed082f5

@davehunt davehunt closed this Aug 22, 2013

Contributor

whimboo commented Aug 22, 2013

While checking the current state we might also want to show the type of report in the title and not only the instance of the dashboard. I would personally also like if we can cover this in this issue to improve bookmarking even more.

@whimboo whimboo reopened this Aug 22, 2013

Contributor

tojon commented Aug 22, 2013

Sure, I will look into that in a few days, once we feel the improvement already completed is behaving fine in all the dashboard instances. ie. no regressions or performance issues.

It should be possible assuming future menus continue to adhere to the convention which adds a url path suffix:
eg.
endurance/reports/

My only concern for that added functionality is I think it will introduce a lot of redraws to the titlebar/tab when navigating the drop down menus. Whereas now we effectively have none. That and the cost of the calculation to build it, each time a menu is traversed.

It's a question of benefit of that extra sub-bookmarking vs. user experience/polish. We can try it and see what the experience is like.

Contributor

whimboo commented Aug 22, 2013

I do not propose that we update the title dynamically when you hover over a menu item. Only when you load a new 'virtual' web page and the location changes. That's not different as when we would load a complete new page. The title would always be updated.

Contributor

tojon commented Aug 23, 2013

Right, that is my understanding of the proposal, and the resulting redraw when the URL path suffix changes.

We can try it and see.

Contributor

tojon commented Mar 5, 2014

Here's a couple screen grabs, one using a dash, the other using comma (which takes one less character). I think we will want to create another hash in the config file, mapping the URL suffixes to the desired strings, if you agree on that concept?

Adding @whimboo and @davehunt for feedback below to see if they like the result.

titlesuffixstyle1
titlesuffixstyle2

Contributor

whimboo commented Mar 6, 2014

Usually separation should happen with a dash. So I like that more. Beside that we should always capitalize the first letter similar to the content in the menus. Otherwise that looks fine.

Member

davehunt commented Mar 6, 2014

I agree with @whimboo's comment. Thanks! 😄

Contributor

tojon commented Mar 6, 2014

Ok, will do. Yup, I was just trying that lower case to de-emphasize sub-menus, but will I go with a leading capital.

I would love to try to deduce the new string on the fly directly from the URL ie. without a config hash. But since the URL path components aren't the same as the menu names, eg. "/top/" - it is probably the simplest and fastest compute to go with the config approach where "/top" becomes "Top Failures".

Contributor

whimboo commented Mar 6, 2014

We might need a mapping in the config files which can also be used to populate the template for the menu. That way we would have those variables on a single place only.

Contributor

tojon commented Mar 7, 2014

I got something working locally yesterday in basic form, it looks nice, etc. However, our buildTitle() only gets triggered a page reload. That worked fine when we just switched domains. Not so when using window.location.hash to test for hash path changes within the page itself. As a result the title doesn't update on its own when navigating the sub-menus. I will look at that part next week.

Contributor

tojon commented Mar 20, 2014

As discussed with whimboo on IRC last week, we want to avoid a page reload. We would prefer instead to manipulate the DOM directly when each menu is selected and/or the hash changes. I have started working on that, and am making progress.

Contributor

tojon commented Mar 21, 2014

I've got something that seems to work well locally. It will also handle an unmatched server name, but still build the sub-menu path correctly, and do what we want in all other cases I've tested. I'm going to push the branch to my remote and add further comments there, before opening a PR.

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Mar 21, 2014

Contributor

tojon commented Apr 9, 2014

Just spoke with @whimboo on IRC. Updating the issue as a reminder, and so he can look at the above commit and its accompanying comments.

Contributor

whimboo commented Apr 16, 2014

I did some comments on the commit you were referring. Please check those. I think it looks fine except some minor things to get updated. I would suggest you create a PR with the updates included so we can push it to sandbox. Thanks!

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Apr 16, 2014

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Apr 17, 2014

Contributor

tojon commented Apr 17, 2014

I've made the changes suggested above, and it seems to be working ok so far.

For some reason the PR didn't get automatically referenced. Not sure why, as I had the issue in the subject. Anyway it's #94

Contributor

whimboo commented Apr 22, 2014

For a reference you have to put the number of the issue or PT in a comment.

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Apr 29, 2014

tojon added a commit to tojon/mozmill-dashboard that referenced this issue May 23, 2014

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Jun 4, 2014

tojon added a commit to tojon/mozmill-dashboard that referenced this issue Jun 10, 2014

Contributor

whimboo commented Jun 11, 2014

PR #94 has been merged. Thank you Jonathan!

@whimboo whimboo closed this Jun 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment