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

guide: add beginnings of a user guide #215

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Conversation

oliver-sanders
Copy link
Member

No description provided.

@oliver-sanders oliver-sanders self-assigned this Sep 5, 2019
@codecov-io
Copy link

codecov-io commented Sep 5, 2019

Codecov Report

Merging #215 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #215   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          16     16           
  Lines         226    226           
=====================================
  Misses        226    226

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 019cda2...0aa1ca8. Read the comment docs.

@wxtim wxtim self-requested a review September 5, 2019 13:09
@wxtim
Copy link
Member

wxtim commented Sep 5, 2019

I notice that the "special task states" doesn't scale to full width on smallish screens. On very old phones it might look even worse:
Screen Shot 2019-09-05 at 14 52 26

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Nothing other than the comment about the styling. I think it would probably be better sorted out now rather than later, but is low priority.

@sadielbartholomew
Copy link
Contributor

#155 is the Issue I mentioned in the offline discussions on this just now. In short, ideally we can incorporate user guidance into the UI via means of e.g. a tour or (carefully designed & placed) tooltips or similar, rather than have it as a separate tab or page, where it essentially becomes documentation in my eyes. And I don't think any good UI should need documentation!

@hjoliver
Copy link
Member

hjoliver commented Sep 5, 2019

@sadielbartholomew -

And I don't think any good UI should need documentation!

Maybe you're right in principle, but I tend to think we should have a documentation page as well in case we fail to quite live up to that lofty goal, and besides I think at least some users might prefer a minimal documentation page with all the necessary information in one place - e.g. task state meanings, what the various views are good for, etc. Plus some of this information isn't strictly "UI" - it's about the meaning of the data that the UI displays (maybe we could replace that with tool-tips or similar ... but that might be annoying; or an optional UI "tour" ... but that would likely need to be a future enhancement?).

So I'd suggest merge this (when it passes review), and add to it as we go, and consider removing it in the future if our UI turns out to be so staggeringly intuitive that the documentation isn't needed! Is that OK with you?

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM, apart from the problem mentioned by @wxtim. Which seems to be caused by this CSS (the values should be 100%; not sure how to fix though as it's not part of this branch):
Screenshot from 2019-09-06 11-55-32

src/views/Guide.vue Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

hjoliver commented Sep 6, 2019

Also: need a link from the dashboard page?

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(Having made two suggestions for changes, I suppose I should revoke my initial approval!)

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Will leave merging up to @oliver-sanders. We can merge as-is and open other PR's to link to the page from the dashboard (I think that's where the link will come from?), and address my comments. Or make further changes here, either way is fine for me as this is new code, doesn't break anything so can be merged with no issues 👍

And really liked being able to see the job and task states side by side. As well as having a place for the special states 🎉

src/views/Guide.vue Show resolved Hide resolved
src/views/Guide.vue Outdated Show resolved Hide resolved
src/views/Guide.vue Outdated Show resolved Hide resolved
src/views/Guide.vue Outdated Show resolved Hide resolved
src/views/Guide.vue Outdated Show resolved Hide resolved
src/views/Guide.vue Outdated Show resolved Hide resolved
color: $grey;
}
}
</style>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use Vuetify typography and flex grid (will change a bit when we have vuetify 2)?

Removing the styles from here, and using the code from this PR oliver-sanders#1

  • Before & after diff desktop:

image

image

  • Before & after diff mobile:

image

image

Might still need some adjustments, but that would be easier to be adjusted to Vuetify 2 too (WIP #11).

(Also added some review comments showing what I did basically)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can use Vuetify typography and flex grid (will change a bit when we have vuetify 2)?

Any reason to change from using a table? Happy to use Vuetify components if they are higher quality, but in this case I don't think they are.

Copy link
Member

Choose a reason for hiding this comment

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

No special reason. I only used these Vuetify components because they translate to flex, flex-wrap, nowrap, and flex columns with %s, etc, and I was using them in the Tree view recently. I can try to achieve the same with a table 👍

@kinow
Copy link
Member

kinow commented Sep 6, 2019

@hjoliver sorry, started reviewing in the morning and didn't see your review until I submitted mine.

Also: need a link from the dashboard page?

There's a TODO for it somewhere I think. But if that's a new link in the dashboard, that will balance the links, and we will have 3 links on each side 😬

@hjoliver
Copy link
Member

hjoliver commented Sep 6, 2019

No need for sorry - you did a proper review! 👍

@oliver-sanders
Copy link
Member Author

I don't think any good UI should need documentation!

I know what you mean, a good UI should just be obvious, but the Cylc UI definitely needs explaining. At present most users are unaware of many of the useful features of the current Cylc GUI so there is definitely a documentation gap we need to cross.

There are good UIs which have documentation e.g. GitHub.

I don't think we should write a user manual for the GUI, perhaps "guide" is the wrong word but there are things like task-job separation which definitely need explaining.

The modern approach to GUI docs seems to be little bitesize chunks of easily digestible information which get slowly dribbled through to the user (very agile). As we add features I think we should write little "cards" to go along with the changes (like the ones in this PR) telling users about the new feature and how it works.

The "guide" page is just the place where these "cards" live. Users don't necessarily browse to this page for help, we could link to a "card" from within the GUI.

To re-use GitHub as an example they have a chengelog with "cards" containing information about new features, users get linked through to the changelog from the UI (they also have a blog for longer reads). The "Guide" page is a more "browsable" version of this changelog providing everything in one place for those motivated souls who want to find out more.

@sadielbartholomew
Copy link
Contributor

So I'd suggest merge this (when it passes review), and add to it as we go, and consider removing it in the future if our UI turns out to be so staggeringly intuitive that the documentation isn't needed! Is that OK with you?

Yes, that's okay by me. Oliver clarified some of the points of the offline discussion we had in his comment above.

I don't think we should write a user manual for the GUI, perhaps "guide" is the wrong word but there are things like task-job separation which definitely need explaining.

I agree that most of the aspects that will need explaining for certain relate to new facets such as task & job separation. And whilst these are represented by state icons etc., ultimately they are not pure UI aspects but Cylc logic ones.

I think at least some users might prefer a minimal documentation page with all the necessary information in one place - e.g. task state meanings, what the various views are good for, etc.

Fair enough, & probably true, but in that case I would wonder why it should go in a tab of the UI & not in the documentation proper. In fact, I think as a UI tab we might risk wasting a significant amount of UI server resource, as users might open a dedicated tab just for reading the guide (or even multiple ones 😟) & flick to-&-from it for advice? If necessary could we mitigate the server load for the guide endpoint in some way, maybe?

I'm not going to push for a different approach, but I'm raising some conerns 😁! Certainly for now this is good enough way to incorporate the guidance content (which looks great so far, BTW).

@kinow kinow added this to the 0.2 milestone Sep 14, 2019
@hjoliver hjoliver merged commit c29307f into cylc:master Sep 25, 2019
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.

6 participants