-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Feat] Add feedback and about to viewer #1115
base: develop
Are you sure you want to change the base?
Conversation
UI unit Tests12 tests 12 ✅ 0s ⏱️ Results for commit 4592a0a. ♻️ This comment has been updated with latest results. |
926410a
to
b32b327
Compare
b32b327
to
5dda76f
Compare
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 good, I left 1 request and 1 comment.
import NewTabLinkRenderer from './NewTabLinkRenderer.svelte'; | ||
import { onMount } from 'svelte'; | ||
|
||
export let text: Readable<string>; |
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.
I think it would be simpler to just pass the string in directly (not wrapped in a store), if the caller has a store they can unwrap it when passing the value to the dialog
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.
The reason for the store is that it's a UI string, so it should theoretically be translatable. I.e. the result of t('viewer.about_message')
is a store, so that the UI can theoretically be retranslated on demand.
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.
So, I'd opt for leaving it a store. Maybe I'll put it in en.json where it belongs.
let toggle: Toggle; | ||
|
||
onMount(() => { | ||
if (!localStorage.getItem('suppressAbout')) { |
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 will work for the viewer, however it doesn't work for FW Lite as local storage does not persist for the moment
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 one is the "comment", right?
That's a good point 😕.
Can we tackle that problem when it becomes a problem?
use the version from the assembly as the feedback version
pushed my own change to modify the version being used when reporting feedback |
Add's the feedback button to the viewer as well as an about button + dialog.
The dialog appears automatically on a user's first visit to the viewer.
Then there's a funky commit: b725184
We're currently embedding global styles (e.g. tailwind and any other global styles) in a
<style global>
element in the root component. Unfortunately, it seems that Svelte's preprocessing is pretty fragile when it comes to using global styles. So, that approach has forced me to use various workarounds (a funky style rewrite hack to get some dark-mode styles to work, splitting up style declarations, making more styles global than actually desired) and finally it totally blocked me from using the tailwind typography plugin.So, this commit tries another approach that's at least equally as funky, BUT it works 🤷. And remember, this is only the viewer 😉, so it's the only thing using this weird approach where as FWL doesn't use anything funky AND now benefits from not having to write styles in a way that makes global happy.
FYI The viewer is currently broken and being fixed in #1105