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

Display if a doc page has been automated on istio.io #7672

Closed
linsun opened this issue Jul 7, 2020 · 29 comments · Fixed by #7734
Closed

Display if a doc page has been automated on istio.io #7672

linsun opened this issue Jul 7, 2020 · 29 comments · Fixed by #7734

Comments

@linsun
Copy link
Member

linsun commented Jul 7, 2020

@frankbu @brian-avery and doc team has done a lot of good work on istio.io test automation and there are metadata today in each page to indicate if a doc page has been automated with our new test framework. It is desirable to reflect that in the UI itself so users would have the visibility into that info as they try each page.

@qwertyuiop888 has kindly offered to look into this and been prototyping this.

cc @davidhauck

@linsun linsun changed the title Display if a doc page has been automated Display if a doc page has been automated on istio.io Jul 7, 2020
@albertsun0
Copy link
Contributor

This is a great idea. An indicator would help users judge if problems running commands are their mistake, or possibly a problem with documentation. This is a prototype indicator.

Screen Shot 2020-07-10 at 12 20 33 PM

@linsun
Copy link
Member Author

linsun commented Jul 10, 2020

@qwertyuiop888 LG! What does the page that has automated testing look like?

@istio-policy-bot istio-policy-bot added the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Jul 10, 2020
@linsun linsun removed the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Jul 10, 2020
@istio-policy-bot istio-policy-bot added the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Jul 10, 2020
@linsun
Copy link
Member Author

linsun commented Jul 10, 2020

Since you are already working on it :)

@albertsun0
Copy link
Contributor

It would look something like this. Right now it is using the checkmark icon, but it would be better with an icon more similar to the clock icon for reading time.
Screen Shot 2020-07-10 at 5 01 03 PM

@linsun
Copy link
Member Author

linsun commented Jul 13, 2020

@qwertyuiop888 Thanks! I was thinking the first page, it could show "Needs Automation" (which could link to the automation page @frankbu mentioned how to contribute to docs to help automate a page).

For the 2nd page, it could show "Commands Automated".

cc @brian-avery @ericvn for input

@linsun linsun added kind/docs kind/site infra and removed lifecycle/needs-triage Indicates a new PR or issue needs to be triaged labels Jul 13, 2020
@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

@qwertyuiop888 what do you mean by but it would be better with an icon more similar to the clock icon for reading time? I think a green check mark or red x next to word TESTED would be good (no time indicator), since it means that the doc is tested against the release of the docs indicated on top left corner of the page. The word TESTED should be a clickable link that takes users to the doc testing README so that users can find out exactly what it means, and also encourage them to help write tests for docs that are still untested.

@linsun
Copy link
Member Author

linsun commented Jul 13, 2020

@frankbu thanks for the input! I was debating on words TESTED @qwertyuiop888 used. My only concern is we still do manual testing each release, certainly we don't have bandwidth to test each page with each RC or final release build, however many of these unautomated pages are still tested on 1 build that is close to the final release build. That is why I was hesitate to use words NOT TESTED on the page that is not automated yet.

@istio-policy-bot istio-policy-bot added the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Jul 13, 2020
@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

@linsun That's the reason I suggested the word TESTED should be a clickable link that explains exactly what it means. Saying something like "Commands Automated" is just more noise, but will still be totally unclear what that means, without a link. Keeping it as simple as possible (just one word (TESTED), a clickable link with a checkmark or x next to it would be best IMO.

We could improve it more by adding hover help over the word TESTED that gives a short summary (e.g., An automated test is available for this document) of what it means.

@linsun
Copy link
Member Author

linsun commented Jul 13, 2020

@frankbu I think we are saying the same thing. Clearly, it needs some explanation of the word(s) chosen. I was more concerned with the pages that needs tests because I think that is the page users will have questions. I don't often question things when there is a green checkbox. :)

I'm good with 1 word TESTED with a link to explain further. Hover the word to show a quick explanation is an interesting idea!

@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

Btw, a tested doc is still not a 100% guarantee that it will work. There can be errors in the text portion of the doc that still can lead the user to run the wrong command, for example. Also, since we don't run the automated test on every platform, it could still be failing on the user's platform. We just need to be clear what exactly TESTED yes/no on the page means. I think the most value we might get out of this, is people deciding to help write tests for untested documents.

@albertsun0
Copy link
Contributor

Thanks Frank and Lin for the feedback! I changed the icon and added the hover summary as well as links. I think this is an easy way to see which pages are tested, and to get users to write tests, especially if they just followed the commands and decided they want to help.

Screen Shot 2020-07-13 at 11 29 53 AM
Screen Shot 2020-07-13 at 11 29 43 AM

@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

That looks pretty good, but don't change the text to NEEDS TESTING. Having a red x next to TESTED basically says that this doc doesn't have an automated test, which readers will understand when they click on the link. Saying NEEDS TESTING is too harsh, it makes it sound like this is some kind of preliminary doc that nobody has even tried yet, which it is not. We want users to understand that an automated test for this page doesn't exist yet (which x TESTED implies), but we don't want users to assume that this doc isn't worth reading because it's totally untested, which NEEDS TESTING implies (Lin's point earlier).

@davidhauck
Copy link
Member

Could we say something like "Tested by hand?"

@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

@davidhauck I think that should be explained after clicking on the link, for users that wonder: what does the red x or greed checkmark mean? The hover help is also a good place to make it clear that TESTED means automated test case exists - yes or no. @qwertyuiop888, what are you currently proposing for the hover help? A nice clear sentence there will help to make sure people aren't confused about this feature.

@linsun
Copy link
Member Author

linsun commented Jul 13, 2020

I think I prefer a help or warning icon for needs testing. red X could be quite concerning.

Alternatively, we only show icon for pages that have been automated. That icon can help users to gain confidence in that page.

@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

I think encouraging people to help writing tests may be the biggest benefit of this feature, so I definitely don't want to only show the icon for pages that have already been automated.

That said, good point, that a red x might be too extreme, since red x usually means something is broken. Maybe a black/white/grey check or x would be better? This is definitely something that I think we might all have different opinions on. It would be nice if we could get some usability experts to review and tell us what color would be best.

@frankbu
Copy link
Collaborator

frankbu commented Jul 13, 2020

A grey checkmark would be pretty intuitive - that means (basic) tested by hand, while green means (better) automated test. The hover help makes it clear we would prefer automated tests, so people might be inclined to help.

@linsun
Copy link
Member Author

linsun commented Jul 13, 2020

A grey checkmark, I like it :). seems pretty intuitive it is limited or manual testing. Along with link/hover over texts to explain further, it will be pretty clear.

@albertsun0
Copy link
Contributor

The grey checkmark idea sounds good. So a grey checkmark for not automated and a green one for automated. The hover text for not automated could be "this document has only been tested by hand. Help create an automated test" and for tested, like you said, "An automated test is available for this document"

@albertsun0
Copy link
Contributor

This is what it looks like.

Screen Shot 2020-07-14 at 2 56 01 PM

Screen Shot 2020-07-14 at 2 55 53 PM

@albertsun0
Copy link
Contributor

Also these are the colors on dark theme. It is much easier to tell the difference between the two checks here.
Screen Shot 2020-07-14 at 3 23 14 PM
Screen Shot 2020-07-14 at 3 23 10 PM

@frankbu
Copy link
Collaborator

frankbu commented Jul 14, 2020

Black background looks great, but the grey check doesn't look so different from the green one on white background. Can you make the grey check lighter, or maybe thinner?

@linsun
Copy link
Member Author

linsun commented Jul 14, 2020

I like them both, thank you @qwertyuiop888 for trying it on both backgrounds!

@frankbu
Copy link
Collaborator

frankbu commented Jul 14, 2020

Maybe you should use a different icon for the grey case. Is there something that would mean "sort of"? I think the exact same icon just with different color might be a problem because it doesn't work for people that are color blind. If we use grey and green check, it really needs to look much lighter (sort of like the difference it black text vs greyed out text).

@albertsun0
Copy link
Contributor

I will look for other icons. This is what it looks like with a lighter check.
Screen Shot 2020-07-14 at 5 13 15 PM
Screen Shot 2020-07-14 at 5 13 06 PM

@frankbu
Copy link
Collaborator

frankbu commented Jul 15, 2020

The light grey looks great on white background, but on black background it really stands out, maybe even more than green, which is the opposite of what we want :-) Unless you can find a good replacement icon, I would suggest just going ahead with the darker grey implementation and send a PR. Other people will review it then, UI and usability designers will likely have suggestions, since they lots of expertise in this area, i.e., things like support for color blind people and other things we haven't even thought of.

So when you implement this, I assume the plan is to show green check for docs with test: yes, grey check for test: no. What about files with test: n/a or no test attribute at all? IMO it's probably best to not show the TESTED tag at all for those docs.

@linsun
Copy link
Member Author

linsun commented Jul 15, 2020

+1 on this.

What about files with test: n/a or no test attribute at all? IMO it's probably best to not show the TESTED tag at all for those docs.

@albertsun0
Copy link
Contributor

The files without the tag do not show it. I'll fix up the code and create a pr in a bit. Thank You Frank and Lin for the feedback!

@christian-posta
Copy link
Contributor

It's not clear, or maybe i missed it... where or how those automated tests get run? Do they get run for every build/release of the docs? If i click on the link it just takes me to the test repository but i don't see test (just instructions for how one could generate them). Can we clarify somewhere what happens with the tests or where are they and what their role is in publishing the docs?

@istio-policy-bot istio-policy-bot removed the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants