-
Notifications
You must be signed in to change notification settings - Fork 218
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
Add preferences for analytics #843
Conversation
Full-stack documentation: Ready https://WordPress.github.io/openverse/_preview/843 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really, really impressive. I have some ideas, particularly around making /preferences
a user-facing page in production and hiding some of the implementation details from end users of our flagging setup:
- We should sentence case the flag names, so
new_header
becomes the more human-readable "New header". - In production, we should only show features which are switchable, since they are the only thing actionable to users. This also allows us to have "private" features in beta. For example, I've been thinking of creating a feature flag which randomly makes 50% of returned results marked as "sensitive", so we can more easily test sensitive content blurring. I would want that to be switchable in staging but hidden entirely in production, because it would only serve to confuse users.
- Similarly, we should hide the 'content-filtering' and 'store-state' sections in production.
- I quite like the idea of a dedicated group for analytics. Could we then exclude analytics from the 'Switchable features' section? And if we show it in production, maybe give it a friendlier name like "Labs" or "Beta features", something like that?
Bugs, other thoughts
-
I believe the store state needs to be stringified, as I'm currently only seeing "[object Object]" output in the code block.
-
What is the "Content filtering" section, exactly? I'm having trouble understanding it. It seems like it's meant to show the state of some flags.
-
The toggle, despite being from the WordPress design language, is difficult visually. The active state is filled in black completely and looks odd to me. Is this how it's meant to look?
One I want to mention is that the preferences page is still internal and not for public use. What I meant by the UI updated to match the mockups is only regarding this segment: Francisco's mockups in #791 suggest a popover for settings and this area matches the contents of that popup. So while you're raising some very good points in the first half of your review, they don't really apply here because making this page public is not the plan here. Bugs, other thoughts
It is very weird that you're seeing UI bugs which are rendering fine for me. Screenshot attached for reference: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh god, I had JS disabled on the page when reviewing the first time 🤦 .
I tested the a11y of the checkbox and I think it's good. Especially considering this is not intended to be a public page, I think we can save additional auditing of that component for the future. LGTM.
Upped the priority to high because I'd like to be able to use this work in #862. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks beautiful! I have a question and a suggestion inline, but they are not blocking.
Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>
Fixes
Fixes #821 by @dhruvkb
Description
This PR
Testing Instructions
Checklist
Update index.md
).main
) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin