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

feat(editor): Passes all codeMirror options to editor #662

Merged
merged 29 commits into from Dec 19, 2017
Merged

feat(editor): Passes all codeMirror options to editor #662

merged 29 commits into from Dec 19, 2017

Conversation

SaraVieira
Copy link
Collaborator

All you now pass inside codeMirrorOptions in your styleguide config will be passed to codeMirror.
I left highlight theme out because I didn't want to cause a breaking change

closes #648

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

Please have a look at my feedback to #649, most of the comments are valid here too.

@SaraVieira
Copy link
Collaborator Author

This is a breaking change, we should keep the old option until the next major release. See updateExample option in the config schema.

I left the option there :)

editorConfig would be a more generic name, and better match existing options like compilerConfig.

Okay , makes sense

@sapegin
Copy link
Member

sapegin commented Nov 6, 2017

I left the option there :)

I still think we should add a deprecation warning, it’s not good to have two valid ways of changing one option.

@sapegin sapegin added this to the 6.1.0 milestone Nov 7, 2017
@SaraVieira
Copy link
Collaborator Author

Name has been changed sir !

How should I add that warning ? :)

@sapegin
Copy link
Member

sapegin commented Nov 9, 2017

You need to add deprecated field with a recommendation for a new option:

} else if (props.deprecated) {
logger.warn(`${chalk.bold(key)} config option is deprecated. ${props.deprecated}`);

@sapegin
Copy link
Member

sapegin commented Nov 9, 2017

You also need to merge the old option with the new object in the config, we shouldn’t access the old option anywhere else.

@SaraVieira
Copy link
Collaborator Author

Yellow !

There was a problem with the logger so I ended up using chalk with a yellow color it looks like this:

screen shot 2017-11-12 at 15 40 11

@@ -89,7 +88,8 @@ module.exports = function sanitizeConfig(config, schema, rootDir) {
throw new StyleguidistError(message);
}
} else if (props.deprecated) {
logger.warn(`${chalk.bold(key)} config option is deprecated. ${props.deprecated}`);
// eslint-disable-next-line no-console
console.log(chalk.yellow.bold(`${key} config option is deprecated. ${props.deprecated}`));
Copy link
Member

@sapegin sapegin Nov 12, 2017

Choose a reason for hiding this comment

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

What was the problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It simply did not log anything

Copy link
Member

Choose a reason for hiding this comment

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

We need to find what’s the issue.

Copy link
Member

Choose a reason for hiding this comment

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

It was actually a bug, I’ve fixed it in master and updated your branch — should work now.

lineWrapping: true,
smartIndent: false,
matchBrackets: true,
viewportMargin: Infinity,
lineNumbers: false,
theme: 'base16-light',
Copy link
Member

Choose a reason for hiding this comment

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

Now we have the same defaults object two times. Keep it in the config only and merge using process method.

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'm sorry , what do you mean ? :(

Too many words

Copy link
Member

Choose a reason for hiding this comment

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

  1. You have this object twice: here and in the config schema. We need only one in the schema.
  2. Check how process method is used in the schema, you need something similar. You also need it to merge the old option with the new one.

@@ -46,9 +47,10 @@ export default class Editor extends Component {

render() {
const { code } = this.props;
const { highlightTheme } = this.context.config;
const { highlightTheme, editorConfig } = this.context.config;
Copy link
Member

Choose a reason for hiding this comment

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

We should not use highlightTheme anywhere except the config schema. Now you’re overwriting the right option with the wrong one anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I thought this is where you meant fpr me to merge

Copy link
Member

Choose a reason for hiding this comment

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

You also need to merge the old option with the new object in the config, we shouldn’t access the old option anywhere else.

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

Merging #662 into master will decrease coverage by 0.14%.
The diff coverage is 88.88%.

Impacted Files Coverage Δ
loaders/styleguide-loader.js 100% <ø> (ø) ⬆️
scripts/utils/sanitizeConfig.js 100% <100%> (ø) ⬆️
scripts/schemas/config.js 85.96% <100%> (+1.05%) ⬆️
src/rsg-components/Editor/Editor.js 85.71% <100%> (-1.79%) ⬇️
scripts/logger.js 76.92% <50%> (+1.92%) ⬆️
src/utils/getRouteData.js 100% <0%> (ø) ⬆️
src/utils/renderStyleguide.js 75% <0%> (ø)

@sapegin
Copy link
Member

sapegin commented Nov 24, 2017

@SaraVieira Hey Sara, any plans to finish? It’s almost done 🦄

@SaraVieira
Copy link
Collaborator Author

Hello ! Sorry for the time it passed :(

I was in london and super busy :(

It's not repeated anymore :)

About the process .... should it get the value i+and if none is return the hightlight theme one and the defaults ?

@sapegin
Copy link
Member

sapegin commented Dec 18, 2017

  1. Merge value of highlightTheme option to theme field.
  2. Merge value of editorConfig with default values object (it shouldn’t be in default field because we want to merge user options with defaults, not overwrite defaults).

Something like this:

editorConfig: {
  // ...
  process: (value, config) => {
    return Object.assign({}, { theme: config.highlightTheme }, value);
  },
},

@SaraVieira
Copy link
Collaborator Author

Like this ?

@sapegin
Copy link
Member

sapegin commented Dec 18, 2017

Almost: defaults should be in the schema.

@SaraVieira
Copy link
Collaborator Author

The CI can't find jest :(

@sapegin
Copy link
Member

sapegin commented Dec 19, 2017

We need to find what’s the issue because it works in master.

@SaraVieira
Copy link
Collaborator Author

I know , the weirdest is that I didn't change anything on any files that have anything to do with config :(

@SaraVieira
Copy link
Collaborator Author

@sapegin did you do anything ? The tests are now passing

@sapegin
Copy link
Member

sapegin commented Dec 19, 2017

I’ve cleared Travis cache and restarted them.

@sapegin
Copy link
Member

sapegin commented Dec 19, 2017

The very last thing: docs! 📖

@@ -227,11 +227,22 @@ module.exports = {
}
```

#### `highlightTheme`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doc changes here :)


Type: `String`, default: `base16-light`
Type: `Object`, default:
```
Copy link
Member

Choose a reason for hiding this comment

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

I’d put a link to the source instead of an object.

@@ -17,7 +17,7 @@ By default, Styleguidist will look for `styleguide.config.js` file in your proje
* [`getComponentPathLine`](#getcomponentpathline)
* [`getExampleFilename`](#getexamplefilename)
* [`handlers`](#handlers)
* [`highlightTheme`](#highlighttheme)
* [`editorConfig`](#editorConfig)
Copy link
Member

Choose a reason for hiding this comment

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

They aren’t ordered now.


[CodeMirror theme](http://codemirror.net/demo/theme.html) name to use for syntax highlighting in the editor.
This will determine how your editor looks, you can check [here](https://codemirror.net/doc/manual.html#config) all the available options
Copy link
Member

Choose a reason for hiding this comment

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

Shorter at least:

Source code editor options, see CodeMirror docs for all available options.

@SaraVieira
Copy link
Collaborator Author

Docs updated! Sorry , didn't notice it was in alphabetic order 🤦‍♀️

@@ -168,6 +168,12 @@ module.exports = {
}
```

#### `editorConfig`

Type: `Object`, default: [scripts/schemas/config.js](https://github.com/styleguidist/react-styleguidist/blob/87efdde2d52209e9c4072368402d11030943d69f/scripts/schemas/config.js#L101)
Copy link
Member

Choose a reason for hiding this comment

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

Better to point to master in case we change the options.

@SaraVieira
Copy link
Collaborator Author

That was dumb of me 🤦‍♀️

Done !! 🙂

@sapegin
Copy link
Member

sapegin commented Dec 19, 2017

Awesome! Praying for CI again 🙏

@SaraVieira
Copy link
Collaborator Author

Weird, some links are failing in the snapshots only on node 4

@sapegin
Copy link
Member

sapegin commented Dec 19, 2017

Yup, that’s very weird. We have that in master too. Restart often helps.

#725 (comment)

@sapegin sapegin merged commit 5a5a376 into master Dec 19, 2017
@sapegin sapegin deleted the 648 branch December 19, 2017 14:56
@sapegin
Copy link
Member

sapegin commented Dec 19, 2017

Merged! :shipit:

@SaraVieira
Copy link
Collaborator Author

YES 🌈

@sapegin sapegin modified the milestones: 6.1.0, 6.2.0 Dec 19, 2017
sapegin added a commit that referenced this pull request Jan 21, 2018
## New features

* New option `editorConfig` to change CodeMirror options (#648, [#662](#662) by @SaraVieira)
* Allow descriptions for sections ([#743](#743) by @SaraVieira)
* Add TypeScript files to default components and ignore patterns ([#749](#749), part of [#750](#750))

## Bug fixes

* Fix global access to all components in isolation mode ([#738](#738))
* Fix color issue in Safari ([#739](#739))
* Allow overriding of renderer-only components ([#710](#710))
* Do not overflow floated elements in examples (#772, [#773](#773) by @roblevintennis)
* HTML escaping in Add example block ([#741](#741))
* Fix infinite loop caused by markdown-to-jsx ([#742](#742))
sapegin added a commit that referenced this pull request Jan 21, 2018
## New features

* New option `editorConfig` to change CodeMirror options (#648, [#662](#662) by @SaraVieira)
* Allow descriptions for sections ([#743](#743) by @SaraVieira)
* Add TypeScript files to default components and ignore patterns ([#749](#749), part of [#750](#750))

## Bug fixes

* Fix global access to all components in isolation mode ([#738](#738))
* Fix color issue in Safari ([#739](#739))
* Allow overriding of renderer-only components ([#710](#710))
* Do not overflow floated elements in examples (#772, [#773](#773) by @roblevintennis)
* HTML escaping in Add example block ([#741](#741))
* Fix infinite loop caused by markdown-to-jsx ([#742](#742))
sapegin added a commit that referenced this pull request Jan 21, 2018
## New features

* New option `editorConfig` to change CodeMirror options (#648, [#662](#662) by @SaraVieira)
* Allow descriptions for sections ([#743](#743) by @SaraVieira)
* Add TypeScript files to default components and ignore patterns ([#749](#749), part of [#750](#750))

## Bug fixes

* Fix global access to all components in isolation mode ([#738](#738))
* Fix color issue in Safari ([#739](#739))
* Allow overriding of renderer-only components ([#710](#710))
* Do not overflow floated elements in examples (#772, [#773](#773) by @roblevintennis)
* HTML escaping in Add example block ([#741](#741))
* Fix infinite loop caused by markdown-to-jsx ([#742](#742))
artem0723 pushed a commit to artem0723/React-Styleguidist that referenced this pull request Oct 28, 2021
## New features

* New option `editorConfig` to change CodeMirror options (#648, [#662](styleguidist/react-styleguidist#662) by @SaraVieira)
* Allow descriptions for sections ([#743](styleguidist/react-styleguidist#743) by @SaraVieira)
* Add TypeScript files to default components and ignore patterns ([#749](styleguidist/react-styleguidist#749), part of [#750](styleguidist/react-styleguidist#750))

## Bug fixes

* Fix global access to all components in isolation mode ([#738](styleguidist/react-styleguidist#738))
* Fix color issue in Safari ([#739](styleguidist/react-styleguidist#739))
* Allow overriding of renderer-only components ([#710](styleguidist/react-styleguidist#710))
* Do not overflow floated elements in examples (#772, [#773](styleguidist/react-styleguidist#773) by @roblevintennis)
* HTML escaping in Add example block ([#741](styleguidist/react-styleguidist#741))
* Fix infinite loop caused by markdown-to-jsx ([#742](styleguidist/react-styleguidist#742))
artem0723 pushed a commit to artem0723/React-Styleguidist that referenced this pull request Oct 28, 2021
## New features

* New option `editorConfig` to change CodeMirror options (#648, [#662](styleguidist/react-styleguidist#662) by @SaraVieira)
* Allow descriptions for sections ([#743](styleguidist/react-styleguidist#743) by @SaraVieira)
* Add TypeScript files to default components and ignore patterns ([#749](styleguidist/react-styleguidist#749), part of [#750](styleguidist/react-styleguidist#750))

## Bug fixes

* Fix global access to all components in isolation mode ([#738](styleguidist/react-styleguidist#738))
* Fix color issue in Safari ([#739](styleguidist/react-styleguidist#739))
* Allow overriding of renderer-only components ([#710](styleguidist/react-styleguidist#710))
* Do not overflow floated elements in examples (#772, [#773](styleguidist/react-styleguidist#773) by @roblevintennis)
* HTML escaping in Add example block ([#741](styleguidist/react-styleguidist#741))
* Fix infinite loop caused by markdown-to-jsx ([#742](styleguidist/react-styleguidist#742))
artem0723 pushed a commit to artem0723/React-Styleguidist that referenced this pull request Oct 28, 2021
## New features

* New option `editorConfig` to change CodeMirror options (#648, [#662](styleguidist/react-styleguidist#662) by @SaraVieira)
* Allow descriptions for sections ([#743](styleguidist/react-styleguidist#743) by @SaraVieira)
* Add TypeScript files to default components and ignore patterns ([#749](styleguidist/react-styleguidist#749), part of [#750](styleguidist/react-styleguidist#750))

## Bug fixes

* Fix global access to all components in isolation mode ([#738](styleguidist/react-styleguidist#738))
* Fix color issue in Safari ([#739](styleguidist/react-styleguidist#739))
* Allow overriding of renderer-only components ([#710](styleguidist/react-styleguidist#710))
* Do not overflow floated elements in examples (#772, [#773](styleguidist/react-styleguidist#773) by @roblevintennis)
* HTML escaping in Add example block ([#741](styleguidist/react-styleguidist#741))
* Fix infinite loop caused by markdown-to-jsx ([#742](styleguidist/react-styleguidist#742))
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.

Line numbers enable configuration needed
3 participants