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

Iterations to link interface #26551

Merged
merged 3 commits into from
Nov 9, 2020
Merged

Iterations to link interface #26551

merged 3 commits into from
Nov 9, 2020

Conversation

karmatosed
Copy link
Member

@karmatosed karmatosed commented Oct 28, 2020

These few changes bring some of the visual iterations explored in #26351 into experimentation. You can see this currently on navigation block for example by adding a link.

Before:

image

After:

image

What changes:

  • Removes the horizontal line break
  • Reduces padding

[ edit of additional changes ]

  • Grid variables
  • Margin changes
  • Small label styling

This doesn't address the icon, as that needs to be confirmed. I am creating it so people can explore what this feels like in an actual pull request.

@karmatosed karmatosed requested a review from jasmussen October 28, 2020 19:39
@karmatosed karmatosed requested a review from ellatrix as a code owner October 28, 2020 19:39
@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Oct 28, 2020
@github-actions
Copy link

github-actions bot commented Oct 28, 2020

Size Change: -932 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/annotations/index.js 3.77 kB -5 B (0%)
build/api-fetch/index.js 3.42 kB -28 B (0%)
build/autop/index.js 2.83 kB -3 B (0%)
build/block-directory/index.js 8.71 kB -10 B (0%)
build/block-editor/index.js 133 kB +62 B (0%)
build/block-editor/style-rtl.css 11.2 kB -28 B (0%)
build/block-editor/style.css 11.1 kB -29 B (0%)
build/block-library/editor-rtl.css 8.96 kB +3 B (0%)
build/block-library/editor.css 8.96 kB +4 B (0%)
build/block-library/index.js 147 kB -45 B (0%)
build/block-library/style-rtl.css 8.05 kB +22 B (0%)
build/block-library/style.css 8.05 kB +21 B (0%)
build/block-serialization-default-parser/index.js 1.87 kB -8 B (0%)
build/block-serialization-spec-parser/index.js 3.06 kB -41 B (1%)
build/blocks/index.js 48 kB -164 B (0%)
build/components/index.js 171 kB -323 B (0%)
build/compose/index.js 9.87 kB +62 B (0%)
build/core-data/index.js 12.5 kB -34 B (0%)
build/data/index.js 8.74 kB -58 B (0%)
build/date/index.js 31.8 kB -13 B (0%)
build/dom/index.js 4.45 kB -2 B (0%)
build/edit-navigation/index.js 11.1 kB -46 B (0%)
build/edit-post/index.js 305 kB -94 B (0%)
build/edit-post/style-rtl.css 6.41 kB +5 B (0%)
build/edit-post/style.css 6.39 kB +1 B
build/edit-site/index.js 22.5 kB -13 B (0%)
build/edit-site/style-rtl.css 3.91 kB -3 B (0%)
build/edit-site/style.css 3.91 kB -4 B (0%)
build/edit-widgets/index.js 26.2 kB -114 B (0%)
build/edit-widgets/style-rtl.css 3.13 kB +3 B (0%)
build/edit-widgets/style.css 3.13 kB +4 B (0%)
build/editor/index.js 42.6 kB -163 B (0%)
build/element/index.js 4.62 kB -27 B (0%)
build/format-library/index.js 6.86 kB +226 B (3%)
build/hooks/index.js 2.16 kB -3 B (0%)
build/keycodes/index.js 1.94 kB -1 B
build/list-reusable-blocks/index.js 3.1 kB -6 B (0%)
build/media-utils/index.js 5.32 kB -24 B (0%)
build/notices/index.js 1.77 kB -16 B (0%)
build/nux/index.js 3.4 kB -19 B (0%)
build/plugins/index.js 2.56 kB +2 B (0%)
build/redux-routine/index.js 2.84 kB -9 B (0%)
build/reusable-blocks/index.js 3.05 kB -2 B (0%)
build/rich-text/index.js 13.4 kB -8 B (0%)
build/server-side-render/index.js 2.77 kB -3 B (0%)
build/viewport/index.js 1.84 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/data-controls/index.js 771 B 0 B
build/deprecated/index.js 769 B 0 B
build/dom-ready/index.js 571 B 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Simple and sweet, we need more PRs like these. It seems a good improvement. Before:

before

After:

interface

I left a comment in case you'd like to do a few more tweaks while you're in there, some of those properties feel a bit arbitrary. Like in the following, the margin below the highlighted item doesn't feel "griddy" (12px grid ideally but 8px in absence of that):

Screenshot 2020-10-29 at 09 50 20

Also, this "recently updated" feels weird. I'd make it 13px semibold font to serve as a subheading, and then be flusn on the left side with the input field:

Screenshot 2020-10-29 at 09 50 26

Those are all totally fine to consider separately if you prefer.

@@ -211,7 +211,6 @@ $block-editor-link-control-number-of-actions: 1;

// Separate Create button when following other suggestions.
.components-button + .block-editor-link-control__search-create {
margin-top: 20px;
overflow: visible;
padding: 12px 15px;
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, it might be cool to update these to use variables, such as $grid-unit-15 and $grid-unit-20.

@karmatosed
Copy link
Member Author

I left a comment in case you'd like to do a few more tweaks while you're in there

Love that idea!

I am going to push each one to break them apart. Then I will collect them all in the issue so there's a record of the changes. I have made a variable as there wasn't one for the font size, I think having one could be useful. I could just set it to 13px though if not. Here is the result:

image

@karmatosed karmatosed changed the title Iterations to link interface: reduces padding and removes line Iterations to link interface Oct 29, 2020
@karmatosed
Copy link
Member Author

@jasmussen I just made a commit to this PR that I would love feedback on, that is regarding the removal of gradients on search results. Here is a before and after:

Before:

results

After:

image

I can see what use they are for longer results, but as they interfer with the hover, I am wondering if we could remove them? Or perhaps remove them on hover? I am interested in feedback on this.

@jasmussen
Copy link
Contributor

From #26551 (comment), yeah I think we should set that to 13px.

Also looking at it, removing the left indentation did not work as well as I'd hoped. I think the indentation of the page items below are probably the source of that problem, but in order to keep this PR small we should probably just add back that subheading indentation and look at the indentation overall as a separate task.

@karmatosed
Copy link
Member Author

look at the indentation overall as a separate task.

Sure, that's something I'm happy to explore later on as think a few areas could benefit from analysis over whether they indent or not and having some tightening up around that.

I just added a commit that brings in some more grid variables - great to align a few more while there. It also reverts the indent.

Here is what this now looks like:

image

@jasmussen
Copy link
Contributor

Yep, that's good. Also good to remove that new variable from the stylesheet, they are worth adding occasionally but we should be careful adding too many, as they are meant to be used widely.

@karmatosed
Copy link
Member Author

@jasmussen thanks, did a clean up. I have a few more things to add to this PR before merging too. I'll work on the icon in next pass as have that file now 🙌🏻

@karmatosed
Copy link
Member Author

karmatosed commented Nov 6, 2020

I have added the updates icon. Props to @jameskoster for help finding the location there.

Before

2020-11-06 at 18 44

After

after

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Nov 6, 2020
@karmatosed karmatosed self-assigned this Nov 6, 2020
@karmatosed
Copy link
Member Author

karmatosed commented Nov 6, 2020

One more additional icon change is to bring in the '+' with updated styling.

Before

image

After

image

Further clean-up

This does open up more cleanup potential where the url icon isn't aligning with the +. Here you can see that quite clearly:

image

Feedback

I didn't bring in the background as the adding blocks have, however this does help visually. There could be an argument for that here, so I am going to put on the feedback label again for some thoughts before moving to commit. Here is what that looks like:

image

Next steps

This PR is ready once these icons have feedback to move to code review and potentially commit.

@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Nov 6, 2020
@jasmussen
Copy link
Contributor

When trying to compile this I'm getting a "Error: Undefined variable. ... font-size: $helptext-font-size;`

@jasmussen jasmussen force-pushed the try/link-iterations branch from 0ba79d0 to 23c244e Compare November 9, 2020 10:18
@karmatosed
Copy link
Member Author

I am just trying to get tests to pass but the variable has been updated and some iterations done to icon placement. After that as this had a code review, I will look to head to commit. Thanks everyone for the help in getting here.

@@ -5,7 +5,7 @@ import { SVG, Path } from '@wordpress/primitives';

const keyboardReturn = (
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="-2 -2 24 24">
<Path d="M16 4h2v9H7v3l-5-4 5-4v3h9V4z" />
<Path d="M6.734 16.106l2.176-2.38-1.093-1.028-3.846 4.158 3.846 4.157 1.093-1.027-2.176-2.38h2.811c1.125 0 2.25.03 3.374 0 1.428-.001 3.362-.25 4.963-1.277 1.66-1.065 2.868-2.906 2.868-5.859 0-2.479-1.327-4.896-3.65-5.93-1.82-.813-3.044-.8-4.806-.788l-.567.002v1.5c.184 0 .368 0 .553-.002 1.82-.007 2.704-.014 4.21.657 1.854.827 2.76 2.657 2.76 4.561 0 2.472-.973 3.824-2.178 4.596-1.258.807-2.864 1.04-4.163 1.04h-.02c-1.115.03-2.229 0-3.344 0H6.734z"/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Path d="M6.734 16.106l2.176-2.38-1.093-1.028-3.846 4.158 3.846 4.157 1.093-1.027-2.176-2.38h2.811c1.125 0 2.25.03 3.374 0 1.428-.001 3.362-.25 4.963-1.277 1.66-1.065 2.868-2.906 2.868-5.859 0-2.479-1.327-4.896-3.65-5.93-1.82-.813-3.044-.8-4.806-.788l-.567.002v1.5c.184 0 .368 0 .553-.002 1.82-.007 2.704-.014 4.21.657 1.854.827 2.76 2.657 2.76 4.561 0 2.472-.973 3.824-2.178 4.596-1.258.807-2.864 1.04-4.163 1.04h-.02c-1.115.03-2.229 0-3.344 0H6.734z"/>
<Path d="M6.734 16.106l2.176-2.38-1.093-1.028-3.846 4.158 3.846 4.157 1.093-1.027-2.176-2.38h2.811c1.125 0 2.25.03 3.374 0 1.428-.001 3.362-.25 4.963-1.277 1.66-1.065 2.868-2.906 2.868-5.859 0-2.479-1.327-4.896-3.65-5.93-1.82-.813-3.044-.8-4.806-.788l-.567.002v1.5c.184 0 .368 0 .553-.002 1.82-.007 2.704-.014 4.21.657 1.854.827 2.76 2.657 2.76 4.561 0 2.472-.973 3.824-2.178 4.596-1.258.807-2.864 1.04-4.163 1.04h-.02c-1.115.03-2.229 0-3.344 0H6.734z" />

This should fix a lint error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @ellatrix just pushed a hopeful resolution for that and also ran snapshot.

@karmatosed
Copy link
Member Author

Looks like tests have passed so going to commit this one. Thank you everyone.

@karmatosed karmatosed merged commit 4be4389 into master Nov 9, 2020
@karmatosed karmatosed deleted the try/link-iterations branch November 9, 2020 16:44
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants