-
Notifications
You must be signed in to change notification settings - Fork 158
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: Presentation Mode #208
Conversation
This is kind of hard to test without this issue being resolved first. |
I use this in Figma a lot. I like calling it "Show / Hide UI" that is super clear to me. Also Command + / seems like a good short cut. Love this feature. |
This pull request is automatically being deployed by Amplify Hosting (learn more). |
@davidkircos I don't love the name "Show/Hide UI" because it feels like it breaks uniformity with how we name all our other view preferences, e.g. we already have other view prefs which are labeled "Show ____"
In theory, they're doing the same thing as this, so shouldn't they be called "Show/Hide ____" e.g.
But the "shown" or "hidden" state is communicated by checkmarks in our app, so you don't really need the dual label. For example, this is Figma today: can you tell, from only this screenshot, whether it's showing or hiding the UI? i.e. does the checked box mean it's showing or hiding when the label says both "Show" and "Hide"? But to your point, perhaps its obvious enough when you use the app. The options in my head are:
Typing this out is an exercise in thinking and maybe just "Show/hide UI" is enough and people will just use it without thinking about it too much — but I don't have to love how it breaks consistency in labeling with the other menu items ha |
How about “Show application UI”?
…On Thu, Feb 2, 2023 at 2:56 PM Jim Nielsen ***@***.***> wrote:
@davidkircos <https://github.com/davidkircos> I don't love the name
"Show/Hide UI" because it feels like it breaks uniformity with how we name
all our other view preferences, e.g. we already have other view prefs which
are labeled "Show ____"
- Show row and column headings
- Show grid axis
- Show grid lines
- Show cell type outlines
In theory, they're doing the same thing as this, so shouldn't they be
called "Show/Hide ____" e.g.
- Show/hide row and column headings
- Show/hide grid axis
- Show/hide grid lines
- Show/hide cell type outlines
- Show/hide UI
But the "shown" or "hidden" state is communicated by checkmarks in our
app, so you don't really need the dual label. For example, this is Figma
today: can you tell, from only this screenshot, whether it's showing or
hiding the UI? i.e. does the checked box mean it's showing or hiding when
the label says both "Show" and "Hide"?
[image: CleanShot 2023-02-02 at 14 49 ***@***.***
<https://user-images.githubusercontent.com/1316441/216457358-e3c3d1d7-0de4-4a98-b46a-9af9539683ec.png>
But to your point, perhaps its obvious enough when you use the app.
The options in my head are:
1. Give it a name, like "Zen mode" or "Presentation mode", as it's not
just toggling individual preferences like the others but a grouping of
them. Then you can easily have a checkbox like [X] Presentation mode
means it's on (unchecked means its off)
2. Have it match the pattern of the other labels and maybe give it a
more precise name, like "Show grid UI"
3. Just roll with "Show/Hide UI"
Typing this out is an exercise in thinking and maybe just "Show/hide UI"
is enough and people will just use it without thinking about it too much —
but I don't have to love how it breaks consistency in labeling with the
other menu items ha
—
Reply to this email directly, view it on GitHub
<#208 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2RO7JFLFM4FOJV2TOADRTWVQURVANCNFSM6AAAAAAUPN2CBU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
} | ||
|
||
export const useGridSettings = (): GridSettingsReturn => { | ||
const [settings, setSettings] = useLocalStorage('gridSettings', defaultGridSettings); | ||
const [showDebugMenu, setShowDebugMenu] = useLocalStorage('showDebugMenu', false); |
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'm unsure what to think about this....
The file is named useGridSettings
so it's about settings in the grid.
But I think, more generically, it's really about "View" settings because "Hide application UI", at least in my mind, should control showing/hiding the "Debug menu" (which, I think, is not technically a "grid setting" because it lives at the level of the application).
So that's a long way to say: I think this code works, but maybe is starting to break out of its original abstraction. Not sure if that's something we should consider refactoring now, or just leave it be and roll with it. I tried to follow the conventions in this file.
settings.showCellTypeOutlines || | ||
showDebugMenu | ||
); | ||
}, [settings]); |
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 derived state, so i'm returning it as a function you have to call to get the boolean representation of state vs. an actual boolean.
} | ||
|
||
emitGridSettingsEvent(); | ||
}, [setSettings, hideApplicationUI, emitGridSettingsEvent, showDebugMenu, setShowDebugMenu]); |
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 wanted each place that consumes this function to not have to worry about passing a value. They can just say "whatever state it's in, toggle it" then they don't have to get the state and figure it out themselves in each place.
It breaks the convention for the other pieces of state in this file, but maybe that's ok because it's not a piece of stored state anyway (it's derived state).
@jimniels I rewrote this to work as Show/Hide UI override, so it saves your other settings and just overrides Show/Hide UI |
@davidkircos this is great. Definitely better to have "show/hide UI" be an override of your current settings vs. a replacement/reset of them. That's a nice addition. A couple of thoughts on the current state of this. Keyboard shortcutsWe still have a clash with keyboard shortcuts. Both "Clear formatting" and "Show/hide UI" are It looks like you might've changed clear formatting to Which is what the command palette says for 'show/hide ui' This is my two cents:
Figma probably uses
Naming is hardI still don't love the name, "Show/Hide UI". When you represent it as a state, it makes no sense which state you're in. For example look at this: Which state is that in? Am I showing the UI or hiding it? What am I turning on or off? Showing the UI? Or hiding it? You can't tell from that toggle. I like calling it "Hide UI" or "Hide interface". This is what Sketch calls it I like it because it makes it clear when you represent the state, what state you're in: But also I like that it feels named to match its function, e.g. "Whatever state the UI is in based on my preferences, override that and hide the application interface". The name of the feature goes with the grain of its function. I like that. (That said, if we do go with "Show/Hide UI" we should keep sentence case consistent, so "Show/hide UI" instead of "Show/Hide UI") |
Interestingly, Figma does "Show/Hide UI", and all their other settings are sentence case. I like this 🤷 For shortcuts, Command + / makes sense for Show/Hide UI, and Command + \ makes sense for clear formatting. That remains consistent with Sheets |
@jimniels I updated all language to refer to it as "Presentation Mode" let me know what you think |
} = options; | ||
|
||
if (!viewport || event.altKey) return false; | ||
|
||
if ((event.metaKey || event.ctrlKey) && event.code === 'KeyP') { | ||
if ((event.metaKey || event.ctrlKey) && (event.code === 'KeyP' || event.code === 'KeyK' || event.code === 'Slash')) { |
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.
Niiiice.
Trying an additional enhancement on this, see #326 |
…reen feat: make presentation mode go full screen
since we hide the header, no need to add this
Notes:
CMD
.
orCMD
\
CMD
K
Z
\
is used as "clear formatting" in SheetsTodos:
CMD
+\
shortcut for "Hide application UI"Maybe rename it to "Show/Hide UI" like figma? Need a little more thought...Will call it "Hide application UI"