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

Command Palette: additional icons / css fix #251

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

waveminded
Copy link
Collaborator

adding more icons to the command palette for consistency, and CSS fix to remove bottom scrollbar empty space on Command Palette

… to remove bottom scrollbar empty space on Command Palatte
@aws-amplify-us-west-2
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-251.de5w5iglj13on.amplifyapp.com

@davidkircos
Copy link
Collaborator

@jimniels could you review this PR?

Copy link
Collaborator

@jimniels jimniels left a comment

Choose a reason for hiding this comment

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

I like the iterative direction towards improvements here. Have a couple thoughts. Let me know what you think...

@@ -2,9 +2,10 @@ import { CommandPaletteListItemSharedProps } from '../CommandPaletteListItem';
import { CommandPaletteListItem } from '../CommandPaletteListItem';
import { copyToClipboard, cutToClipboard, pasteFromClipboard } from '../../../../core/actions/clipboard';
import { KeyboardSymbols } from '../../../../helpers/keyboardSymbols';
import { ContentCopy, ContentPaste, ContentCut, East } from '@mui/icons-material';
import { ContentCopy, ContentPaste, ContentCut, East, UndoOutlined } from '@mui/icons-material';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like UndoOutlined isn't being used here. We should remove it

@@ -91,7 +91,7 @@ export const CommandPalette = (props: Props) => {
/>

<Divider />
<div style={{ maxHeight: '330px', overflow: 'scroll' }}>
<div style={{ maxHeight: '330px', overflowY: 'scroll', paddingBottom: '5px' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch on being more explicit about overflow direction here 👍

@@ -2,13 +2,15 @@ import { CommandPaletteListItemSharedProps } from '../CommandPaletteListItem';
import { CommandPaletteListItem } from '../CommandPaletteListItem';
import { newGridFile, openGridFile } from '../../../../core/actions/gridFile/OpenGridFile';
import { SaveGridFile } from '../../../../core/actions/gridFile/SaveGridFile';
import { UnarchiveOutlined, NoteAddOutlined, SaveAlt } from '@mui/icons-material';
Copy link
Collaborator

@jimniels jimniels Feb 13, 2023

Choose a reason for hiding this comment

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

I'm thinking about these icons a little more...

image

Here's my thoughts, tell me what you think:

I don't love how different these icons are from each other. They're all conceptually about "file" but their icons are all so different. Even the arrows on the icons are differently-shaped arrows

CleanShot 2023-02-13 at 11 01 59@2x

I feel like using the outline of a "file" for each icon would be the most intuitive. It doesn't look like the default material UI has three different "file" icons representing new, save, and open. If we go with just what they have, I would think FileOpenOutlined might be best for open. As for save, maybe the traditional floppy disk SaveOutlined would be best?

image

Even that, I still don't love. Again, I feel like the notion of a "document" icon that is repeated for each would be the most useful. Maybe we could create our own "save" icon that is the same as the existing "FileOpenOutlined" but we reverse the direction of the arrow, e.g.

image

That last one is the one I like the best. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to show you how to create that file too...it's pretty straightforward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dig the last iteration for now. I know the MUI icons aren't the greatest. Which file are you referring? Just manipulating the svg of the icon?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, the "new file" would just be the same icon. "open file" would be the UploadFile icon, and the "save file" would be a custom one we create from the UploadFile icon where we just invert the direction of the arrow.

Copy link
Collaborator

@jimniels jimniels left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@waveminded waveminded changed the base branch from development to main February 14, 2023 17:16
@waveminded waveminded merged commit 599844f into main Feb 14, 2023
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.

None yet

3 participants