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

Docs Feedback widget #13550

Merged
merged 18 commits into from
May 14, 2019
Merged

Docs Feedback widget #13550

merged 18 commits into from
May 14, 2019

Conversation

marcysutton
Copy link
Contributor

@marcysutton marcysutton commented Apr 22, 2019

Description

Introducing a feedback widget to the docs section, with code integrated from @jlengstorf's feedback widget repo. We want to make it easy to collect feedback about docs from users in the moment, and by creating our own widget we will control the data (as opposed to a third-party service).

This PR adds two new dependencies for managing state:

  • xstate
  • @xstate/react

It would be super cool to add some tests for this, primarily to assert focus management in after view changes so we know they continue to work. Hopefully I'll get some time to work on that...but I didn't want to hold up reviewing of this in the meantime.

TODO:

Test with NVDA on IE11 and Firefox. The master branch is currently failing to build for me so I can't test it with Parallels.

@marcysutton marcysutton requested a review from a team as a code owner April 22, 2019 20:44
fk
fk previously requested changes Apr 24, 2019
Copy link
Contributor

@fk fk left a comment

Choose a reason for hiding this comment

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

This looks great, love the emoji animations! 😻

I didn't look at the code much (lacking skills + time), but I have one request: Can we use the design tokens from utils/presets, e.g. use colors.gatsby instead of rebeccapurple, and do the same for margin and padding (space), box-shadow, border-radius, font-size etc.? 🙏

I'm happy to do so myself, but won't be able to fit that in today.
Also — I know those tokens lack documentation, planning on adding that this week. :/

fk and others added 2 commits April 24, 2019 17:26
There’s more to do, but this is a start…

- remove `font-family: sans-serif` along the way
- remove components/feedback-widget/presets in favor of utils/presets.breakpoints
@greglobinski
Copy link
Contributor

I've changed textarea height to fix widget height

before

Screenshot from 2019-04-24 19-28-12

after

Screenshot from 2019-04-24 19-40-48

@fk fk dismissed their stale review April 24, 2019 20:49

I won't manage "Use design tokens (2/2)" today anymore and blocking a merge would be silly. Happy to follow up tomorrow on the token stuff!

@pieh
Copy link
Contributor

pieh commented Apr 25, 2019

For people who want to try it - here's preview https://5cc089e52d11ebeccaf8db72--gatsby-org-pieh-previews.netlify.com/docs/ (just a note - it doesn't contain changes made in last 2 commits by Flo and Greg)

fk added 4 commits April 25, 2019 13:08
- chose to reduce the border around the „emoji toggle buttons“ from 3 to 1px
- add horizontal margin for screens below desktop/lg
@fk
Copy link
Contributor

fk commented Apr 25, 2019

Just pushed the missing design tokens stuff and fixed layout for screens below breakpoints.lg (horizontal whitespace, max-width matching content), and merged master to adjust things for #13388.

@greglobinski I had to do some gymnastics for smaller screens/max-width, and changed the textarea width from 99% to 100%, and removed overflow-y: auto. I think everything is still working fine (checked animations, etc.) — could you take another look though please? Don't want to break anything, and not sure if I am aware of everything that needs checking.

Here's how it looks now:

image

image

image

image

image

@greglobinski
Copy link
Contributor

@fk Looking good, thank you

@prestonso
Copy link
Contributor

prestonso commented Apr 29, 2019

I know we have the ? affordance that doubles as a question mark currently, but there should be a question mark at the end of the phrase as well, especially for wider viewports:

Was this doc helpful to you?          [?]

instead of:

Was this doc helpful to you         [?]

@greglobinski
Copy link
Contributor

Actually, there is a hidden question mark for screen readers

 <ToggleButtonLabel>
              Was this doc helpful to you
              <ScreenReaderText>?</ScreenReaderText>
            </ToggleButtonLabel>

so we can show it if we want.

@marcysutton
Copy link
Contributor Author

marcysutton commented Apr 29, 2019

Adding a visible question mark in the button text makes sense to me.

We should also factor this widget in to the docs internationalization effort–would we hide it for other languages? Or collect feedback and get it translated?

@pieh
Copy link
Contributor

pieh commented May 2, 2019

Few notes about visible extra question mark - I think it looks pretty weird in desktop variant (like double question mark):
Screenshot 2019-05-02 at 18 13 19

It does seem ok for narrow viewports:
Screenshot 2019-05-02 at 18 13 13
Screenshot 2019-05-02 at 18 13 05

I can implement it to show only for narrow viewports but it will feel pretty inconsistent to me. Thoughts?

@greglobinski
Copy link
Contributor

greglobinski commented May 6, 2019

@marcysutton @pieh @prestonso

What about such a solution? We always show a while mark at the end of the question, but instead of the Question mark icon we show a Chat icon?

Screenshot from 2019-05-06 16-27-53
Screenshot from 2019-05-06 16-26-26

@marcysutton
Copy link
Contributor Author

Ooh, I like that! I think using a chat/message icon solves the problem very well.

@fk
Copy link
Contributor

fk commented May 7, 2019

☝️☝️☝️

@muescha
Copy link
Contributor

muescha commented May 9, 2019

Found in screenshot above: is this a one time glitch or is it fixed?
97C96E2E-7694-47E7-B4F5-251C9471DE3B

@greglobinski
Copy link
Contributor

@muescha

Found in screenshot above: is this a one time glitch or is it fixed?

What exactly are you asking about?

@muescha
Copy link
Contributor

muescha commented May 9, 2019

The "poor" icon in this screenshots is shiftet down and not visible

@greglobinski
Copy link
Contributor

greglobinski commented May 9, 2019

The "poor" icon in this screenshots is shiftet down and not visible

@muescha Ok, that's the intentional hover state of the input.

`

export const focusStyle = css`
box-shadow: 0 0 0 0.12rem ${colors.accent};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This focus style still isn't working in Safari. I played around with it a bit and opted to change it to use outline and outline-offset instead:

outline: 2px solid ${colors.accent};
outline-offset: -2px;

https://codepen.io/marcysutton/pen/WBxEom

@marcysutton
Copy link
Contributor Author

marcysutton commented May 11, 2019

This is ready to go now! The only issue I ran into was if you hit the enter key on the smiley radio buttons, it wouldn't go through the submit flow....it would cancel and reset back to the widget toggle button instead. I was able to fix it by adding type="button" to the cancel button. woot!

Also tested it on Windows, and it works great with NVDA, Firefox, and Chrome. Edge was decent but it gets the radio buttons wrong by only announcing "1 of 1" for each instead of "1 of 3", etc. Given they're moving to Chromium I opted to leave that as-is without digging in more.

@muescha
Copy link
Contributor

muescha commented May 12, 2019

Where i can find the server side code repo for the mutation submitFeedback?

@jlengstorf
Copy link
Contributor

@muescha the API source is here. It won't run without setting up a Prisma instance, though — take a look at the .env.EXAMPLE file for details. https://github.com/gatsbyjs/api.gatsbyjs.org/blob/master/src/graphql/public-resolvers.js

@marcysutton
Copy link
Contributor Author

This is ready to go, assuming the form endpoint works. @jlengstorf if you think we need to test that more fully I could use some detailed instructions. Otherwise I think this is ready to merge!

Copy link
Contributor

@jlengstorf jlengstorf left a comment

Choose a reason for hiding this comment

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

🎉

@marcysutton
Copy link
Contributor Author

It works now that I have the API endpoint set up locally (thanks @jlengstorf!). Excited to start collecting data!

@marcysutton marcysutton merged commit ee212ee into master May 14, 2019
@fk
Copy link
Contributor

fk commented May 17, 2019

Yay! 🎉

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.

7 participants