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.

Sign up for GitHub

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

[redesign] editor in new design #2513

Merged
merged 17 commits into from
Jun 29, 2022
Merged

[redesign] editor in new design #2513

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
903c772
add postcss config to allow nesting
thomasheyenbrock Jun 1, 2022
e947193
avoid processing css from `@graphiql/react` with postcss
thomasheyenbrock May 25, 2022
c37be13
add css files for default fonts
thomasheyenbrock May 25, 2022
d149789
add long description with markdown to dev schema
thomasheyenbrock Jun 1, 2022
2c68448
rewrite completion tooltip
thomasheyenbrock May 25, 2022
c47aaac
move editor css to @graphiql/react
thomasheyenbrock Jun 1, 2022
04ef5a0
git mv editor components to `@graphiql/react`
thomasheyenbrock Jun 14, 2022
e1992e7
adjust editor components after moving to `@graphiql/react`
thomasheyenbrock Jun 14, 2022
1cabfbd
move styles for auto-inserted leads
thomasheyenbrock Jun 2, 2022
3ea62ee
make dynamic padding more resilient and add explanatory comment
thomasheyenbrock Jun 14, 2022
cbde0c3
add extra color to avoid conflicts
thomasheyenbrock Jun 14, 2022
db2c34e
colocate markdown and deprecation styles
thomasheyenbrock Jun 14, 2022
8db0e72
add changesets
thomasheyenbrock Jun 21, 2022
1ca5160
fix spell checking
thomasheyenbrock Jun 21, 2022
8c5864c
add changeset for graphiql@2
thomasheyenbrock Jun 24, 2022
c82b22f
don't enter pre-release mode in changeset
thomasheyenbrock Jun 29, 2022
218c261
more elaborate changeset message
thomasheyenbrock Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/five-pillows-fail.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@graphiql/react': minor
---

Add editor components and implement styles for the new GraphiQL design
5 changes: 5 additions & 0 deletions .changeset/gentle-forks-rule.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'codemirror-graphql': major
---

BREAKING: Change the implementation of the info popup when hovering items in the code editor to enable the implementation of the new GraphiQL design
5 changes: 5 additions & 0 deletions .changeset/perfect-flowers-hunt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'graphiql': minor
---

Consume the editor components from `@graphiql/react`
19 changes: 19 additions & 0 deletions .changeset/pre.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"mode": "pre",
"tag": "alpha",
"initialVersions": {
Copy link
Member

Choose a reason for hiding this comment

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

initialVersions here should be graphiql, @graphiql/react, codemirror-graphql and @graphiql/toolkit only.

I imagine the changests CLI added all of these just because they had active diffs or changes, but there is no reason the language service, server, cli, monaco-graphql, vscode-graphql, or any of these examples need to go into pre-release for these changes

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 just gave this a test run locally to see what changeset would do in that case. Turns out that you can't use pre-releases for a subset of packages, it's either all or nothing. So when we'd merge this we'd also force pre-releases on vscode-graphql etc 😕

An alternate way of doing this could be to just have a dedicated branch where we start merging the approved PRs into. Once we have everything we need for v2 we merge this one big PR into main and release it. Wdyt?

Copy link
Member

@acao acao Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

well my first reaction is remembering the pain of dedicated branch we had in 20202 (1.x at the time, and 2.x was main, but then that became the retired rfc branch and 1.x became main again), because then the main and dedicated branch got terribly out of sync! we were issuing 1.x releases from one branch and 2.x from main.

that said, I think your idea will work out fine as long as we don't make any changes to the graphiql packages from main while this branch exists, and the plan is to merge it back into main?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I also don't really favor this approach, but I don't see a better option 😕 (I also hope that we can get all of this done within the next couple of weeks, so this branch won't be around for a longer time 🤞 )

Then let's do it like this! I'll create a branch graphiql-v2 where we'll merge all the breaking changes and the new design into. Once we reviewed and collected everthing we'll merge this branch into main. I'll also rebase the branch regularly so that it won't get out of sync and doesn't block other PRs being merged into main in the meantime.

"example-graphiql-webpack": "1.1.1-alpha.8",
"example-monaco-graphql-react-vite": "0.0.0",
"example-monaco-graphql-webpack": "1.1.1-alpha.7",
"codemirror-graphql": "1.3.2",
"graphiql": "1.9.9",
"@graphiql/react": "0.4.2",
"@graphiql/toolkit": "0.6.0",
"graphql-language-service": "5.0.6",
"graphql-language-service-cli": "3.2.28",
"graphql-language-service-server": "2.7.27",
"monaco-graphql": "1.1.2",
"vscode-graphql": "0.4.13"
},
"changesets": []
}
8 changes: 8 additions & 0 deletions custom-words.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,23 +139,29 @@ eslintignore
filesfor
flowtests
foldgutter
foldmarker
ghapi
graphqlconfig
graphqlrc
graphqls
invalidchar
languageservice
linenumber
linenumbers
linkify
listvalues
matchingbracket
modulemap
myschema
newhope
nocheck
nocursor
nonmatchingbracket
nrtbf
nulltype
nvim
objectvalues
orche
outdir
outlineable
postbuild
Expand All @@ -180,7 +186,9 @@ websockets


// fonts
fira
menlo
roboto

// locations
givatayim
Expand Down
91 changes: 58 additions & 33 deletions packages/codemirror-graphql/src/info.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,36 +74,51 @@ CodeMirror.registerHelper(
(kind === 'Field' && step === 0 && typeInfo.fieldDef) ||
(kind === 'AliasedField' && step === 2 && typeInfo.fieldDef)
) {
const header = document.createElement('div');
header.className = 'CodeMirror-info-header';
renderField(header, typeInfo, options);
const into = document.createElement('div');
renderField(into, typeInfo, options);
into.appendChild(header);
Copy link
Member

Choose a reason for hiding this comment

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

I would like for maintainers such as @yoshiakis and @imolorhe to be able to review this change, to make sure that it doesn't clash with other popular codemirror-graphql implementations outside of graphiql

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! This would be a new major version for codemirror-graphql, so it would not break any dependents. Still good to know if this would be blocking others to update to this new version in any way.

@yoshiakis @imolorhe to sum up the changes for easy digestion:

  • For fields the type prefix was removed, i.e. MyType.myField -> myField
  • For args, the type and field was removed, i.e. MyType.myField(myArg: MyArgType) -> myArg: MyArgType
  • The DOM structure of the info tooltip changed (for styling reasons):
    • The first section (i.e. the clickable parts like type and field name) are wrapped in an additional div
    • The markdown content for deprecation reasons is wrapped in an additional div

Copy link
Member

@acao acao Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

also CCing @stonexer @benjie @sgrove and others who might be interested in this conversation!

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering this is going to be a major version , should not be a problem. Is there a way to tag BREAKING CHANGEs in changeset? If so, this should be tagged as such.

Copy link
Member

Choose a reason for hiding this comment

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

@imolorhe yes, essentially just by making it a major version change and adding the equivalent conventional-commits style breaking change note in bold in the changeset markdown! Then it will appear in the gh release and in the changelog

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this will show up in the changelog! I also made the changeset message more elaborate and added the bullets from my comment above.

renderDescription(into, options, typeInfo.fieldDef as any);
return into;
} else if (kind === 'Directive' && step === 1 && typeInfo.directiveDef) {
const header = document.createElement('div');
header.className = 'CodeMirror-info-header';
renderDirective(header, typeInfo, options);
const into = document.createElement('div');
renderDirective(into, typeInfo, options);
into.appendChild(header);
renderDescription(into, options, typeInfo.directiveDef);
return into;
} else if (kind === 'Argument' && step === 0 && typeInfo.argDef) {
const header = document.createElement('div');
header.className = 'CodeMirror-info-header';
renderArg(header, typeInfo, options);
const into = document.createElement('div');
renderArg(into, typeInfo, options);
into.appendChild(header);
renderDescription(into, options, typeInfo.argDef);
return into;
} else if (
kind === 'EnumValue' &&
typeInfo.enumValue &&
typeInfo.enumValue.description
) {
const header = document.createElement('div');
header.className = 'CodeMirror-info-header';
renderEnumValue(header, typeInfo, options);
const into = document.createElement('div');
renderEnumValue(into, typeInfo, options);
into.appendChild(header);
renderDescription(into, options, typeInfo.enumValue);
return into;
} else if (
kind === 'NamedType' &&
typeInfo.type &&
(typeInfo.type as GraphQLObjectType).description
) {
const header = document.createElement('div');
header.className = 'CodeMirror-info-header';
renderType(header, typeInfo, options, typeInfo.type);
const into = document.createElement('div');
renderType(into, typeInfo, options, typeInfo.type);
into.appendChild(header);
renderDescription(into, options, typeInfo.type);
return into;
}
Expand All @@ -125,10 +140,6 @@ function renderQualifiedField(
options: GraphQLInfoOptions,
) {
const fieldName = typeInfo.fieldDef?.name || '';
if (fieldName.slice(0, 2) !== '__') {
renderType(into, typeInfo, options, typeInfo.parentType);
text(into, '.');
}
text(into, fieldName, 'field-name', options, getFieldReference(typeInfo));
}

Expand All @@ -146,38 +157,47 @@ function renderArg(
typeInfo: TypeInfo,
options: GraphQLInfoOptions,
) {
if (typeInfo.directiveDef) {
renderDirective(into, typeInfo, options);
} else if (typeInfo.fieldDef) {
renderQualifiedField(into, typeInfo, options);
}

const name = typeInfo.argDef?.name || '';
text(into, '(');
text(into, name, 'arg-name', options, getArgumentReference(typeInfo));
renderTypeAnnotation(into, typeInfo, options, typeInfo.inputType);
text(into, ')');
}

function renderTypeAnnotation(
function renderEnumValue(
into: HTMLElement,
typeInfo: TypeInfo,
options: GraphQLInfoOptions,
t: Maybe<GraphQLType>,
) {
text(into, ': ');
renderType(into, typeInfo, options, t);
const name = typeInfo.enumValue?.name || '';
renderType(into, typeInfo, options, typeInfo.inputType);
text(into, '.');
text(into, name, 'enum-value', options, getEnumValueReference(typeInfo));
}

function renderEnumValue(
function renderTypeAnnotation(
into: HTMLElement,
typeInfo: TypeInfo,
options: GraphQLInfoOptions,
t: Maybe<GraphQLType>,
) {
const name = typeInfo.enumValue?.name || '';
renderType(into, typeInfo, options, typeInfo.inputType);
text(into, '.');
text(into, name, 'enum-value', options, getEnumValueReference(typeInfo));
const typeSpan = document.createElement('span');
typeSpan.className = 'type-name-pill';
if (t instanceof GraphQLNonNull) {
renderType(typeSpan, typeInfo, options, t.ofType);
text(typeSpan, '!');
} else if (t instanceof GraphQLList) {
text(typeSpan, '[');
renderType(typeSpan, typeInfo, options, t.ofType);
text(typeSpan, ']');
} else {
text(
typeSpan,
t?.name || '',
'type-name',
options,
getTypeReference(typeInfo, t),
);
}
into.appendChild(typeSpan);
}

function renderType(
Expand Down Expand Up @@ -243,16 +263,21 @@ function renderDeprecation(
if (reason) {
const deprecationDiv = document.createElement('div');
deprecationDiv.className = 'info-deprecation';
into.appendChild(deprecationDiv);

const label = document.createElement('span');
label.className = 'info-deprecation-label';
label.appendChild(document.createTextNode('Deprecated'));
deprecationDiv.appendChild(label);

const reasonDiv = document.createElement('div');
reasonDiv.className = 'info-deprecation-reason';
if (options.renderDescription) {
deprecationDiv.innerHTML = options.renderDescription(reason);
reasonDiv.innerHTML = options.renderDescription(reason);
} else {
deprecationDiv.appendChild(document.createTextNode(reason));
reasonDiv.appendChild(document.createTextNode(reason));
}
const label = document.createElement('span');
label.className = 'info-deprecation-label';
label.appendChild(document.createTextNode('Deprecated: '));
deprecationDiv.insertBefore(label, deprecationDiv.firstChild);
into.appendChild(deprecationDiv);
deprecationDiv.appendChild(reasonDiv);
}
}

Expand Down
58 changes: 58 additions & 0 deletions packages/graphiql-react/font/fira-code.css
View file
Open in desktop

Large diffs are not rendered by default.

272 changes: 272 additions & 0 deletions packages/graphiql-react/font/roboto.css
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions packages/graphiql-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"types": "types/index.d.ts",
"files": [
"dist",
"font",
"src",
"types"
],
Expand All @@ -34,17 +35,16 @@
"codemirror": "^5.65.3",
"codemirror-graphql": "^1.3.2",
"copy-to-clipboard": "^3.2.0",
"escape-html": "^1.0.3",
"graphql-language-service": "^5.0.6",
"markdown-it": "^12.2.0",
"set-value": "^4.1.0"
},
"devDependencies": {
"@types/codemirror": "^5.60.5",
"@types/escape-html": "^1.0.1",
"@types/set-value": "^4.0.1",
"@vitejs/plugin-react": "^1.3.0",
"graphql": "^16.4.0",
"postcss-nesting": "^10.1.7",
"react": "^17.0.2",
"react-dom": "^17.0.2",
"typescript": "^4.6.3",
Expand Down
3 changes: 3 additions & 0 deletions packages/graphiql-react/postcss.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
plugins: [require('postcss-nesting')],
};
Loading