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

Update styles for GraphiQL top bar #3093

Closed
wants to merge 10 commits into from

Conversation

martinlaws
Copy link
Member

@martinlaws martinlaws commented Nov 11, 2023

WHY are these changes introduced?

relates to: #2887 and https://github.com/Shopify/develop-app-management/issues/1494
figma: https://www.figma.com/file/1UrPp4aUxgxSmBOZeJAUkC/GraphiQL?type=design&node-id=5-15794&mode=dev

WHAT is this pull request doing?

This draft PR gets much of the way towards updating the GraphiQL style bar per @matlegault's designs.

in this PR:

  • added encoded SVGs for the following polaris icons: link, running (connected), and disconnected
  • roughed in styles as designed for top bar
  • added Shopify CDN link tag to pull in Inter font, copied font-family rule declaration/fallbacks from web

not in this PR

  • once main is able to build properly again, rebase this PR against those fixes and everything should work properly :)
  • media queries to respond to mobile or tablet viewport sizes
  • implementing a popover for app or store names that cause the pill size to exceed 300px — see: Figma (callout: this might be worthwhile scope to cut, considering we don't have access to the Polaris popover component here)
  • another round of polish

=== UPDATE 2023-11-14 by @amcaplan ===

Build is working, and we made some more improvements. Here's how it looks now.

Before:

Screenshot 2023-11-14 at 21 42 26

After:

Screenshot 2023-11-14 at 21 41 32

How to test your changes?

  1. run Shopify/cli locally on this update-graphiql-top-bar-styles branch
  2. run pnpm shopify app dev --path ../path-to-your-app (if you don't already have an app, you'll be prompted to create one here)
  3. if your app is not already installed in your dev store, please do so now by clicking through the preview link that has been output in the CLI
  4. once dev is running smoothly, press g to launch GraphiQL, you'll see the top bar :)

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/cli-foundations

Copy link
Contributor

github-actions bot commented Nov 11, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.88% (+0% 🔼)
6155/8445
🟡 Branches
70.1% (-0.02% 🔻)
2968/4234
🟡 Functions 71.82% 1570/2186
🟡 Lines
74.02% (+0% 🔼)
5845/7897
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / ConcurrentOutput.tsx
97.62% (-2.38% 🔻)
75% (-8.33% 🔻)
100%
97.44% (-2.56% 🔻)

Test suite run success

1433 tests passing in 664 suites.

Report generated by 🧪jest coverage report action from ca8c8d1

@amcaplan
Copy link
Contributor

Do we really need mobile viewport sizes? GraphiQL really isn't designed for that environment, and I don't think we need to take it into account.

@amcaplan amcaplan force-pushed the update-graphiql-top-bar-styles branch from c853539 to 31ab437 Compare November 13, 2023 10:47
@matlegault
Copy link
Contributor

matlegault commented Nov 13, 2023

Small nitpick (although I also see that a 2nd round of polish would be a different PR) can you vertically center the “link” icon in the badge with the text, and reduce the space between the icon and the text (for both the Status and Link badges)
image

@amcaplan
Copy link
Contributor

@matlegault how's this? Still too much space? Is the alignment OK?

Before:

Screenshot 2023-11-13 at 21 35 31

After:

Screenshot 2023-11-13 at 21 33 48

@matlegault
Copy link
Contributor

matlegault commented Nov 13, 2023

@amcaplan Here if this helps!

Almost there. In general I want the spacing to the left of the icon to be about the same as on the right, visually speaking.

Here, the icon has some padding around the shape itself but if yours doesn't you can aim for about 8px spacing to its left and right 👍
image

@amcaplan
Copy link
Contributor

How's this?

Screenshot 2023-11-14 at 16 58 14 Screenshot 2023-11-14 at 16 58 18

It technically has a bit more spacing on the left, but the rounded corners on the left, plus a tiny bit of space before the letters start, mean that this looks more even than actually balancing the space. Here's without that adjustment, for context:

Screenshot 2023-11-14 at 16 54 07 Screenshot 2023-11-14 at 16 54 22

@amcaplan amcaplan force-pushed the update-graphiql-top-bar-styles branch from f963dce to 7986a72 Compare November 14, 2023 15:00
@amcaplan amcaplan force-pushed the update-graphiql-top-bar-styles branch from 7986a72 to 8a1838e Compare November 14, 2023 19:27
@amcaplan amcaplan marked this pull request as ready for review November 14, 2023 19:38
@amcaplan amcaplan changed the title [DRAFT / WIP] — updating styles for GraphiQL top bar Update styles for GraphiQL top bar Nov 14, 2023

This comment has been minimized.

@martinlaws
Copy link
Member Author

closing this PR in favour of the one @amcaplan merged while I was OOO: #3116

@martinlaws martinlaws closed this Nov 28, 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.

4 participants