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

[EuiMarkdownEditor] Add remark-breaks and line break plugin #5272

Merged
merged 3 commits into from
Oct 20, 2021

Conversation

i-a-n
Copy link
Contributor

@i-a-n i-a-n commented Oct 14, 2021

Summary

This adds the remark-breaks plugin to the default list of plugins for the MarkdownEditor. Now, unless disabled, all single-line linebreaks in the MarkdownEditor will translate to literal line breaks in the preview mode.

Note that multiple consecutive line breaks will still result in only two line breaks, as it does today, so there are no concerns about runaway line breaks making things look weird. Behavior mimics GitHub's comment box.

Checklist

  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

Resolves #5141

[Opening in Draft Mode since this is technically a change in behavior, and I'm not sure if this should be considerd a breaking change since the component is technically in Beta, so I'll be waiting for some feedback on that before merging.]

@i-a-n i-a-n requested a review from miukimiu October 14, 2021 23:24
@i-a-n i-a-n self-assigned this Oct 14, 2021
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5272/

@miukimiu
Copy link
Contributor

miukimiu commented Oct 15, 2021

Thanks @i-a-n for opening this PR,

I also don't see this as a breaking change and we're saying since day one that the markdown editor "uses GitHub flavored markdown". So I guess this behavior should be expected.

For allowing to opt-out the plugin you can see how @chandlerprall did it here: miukimiu/eui@90a669a#diff-d5fec82a5af6bfbe83ee2cee3b7aad4e015f98f57996b8f8eb703a14688efbc0R74.

And we should say in our docs that we can unregister this plugin in the following section:
https://eui.elastic.co/pr_5272/#/editors-syntax/markdown-editor#unregistering-plugins

And update the following demo to show how we can also unregister the line break plugin:

Screenshot 2021-10-15 at 14 42 15

All the rest looks good to me! 👍🏽

@i-a-n
Copy link
Contributor Author

i-a-n commented Oct 18, 2021

thanks for taking a look @miukimiu .

I was going deeper into this one and found a few things:

  1. this line breaks plugin I added is nearly identical in structure to the emoji plugin that's already there... it is imported in the same place (as a getDefaultEuiMarkdownParsingPlugins plugin).
  2. the emoji plugin does not appear to be "unregisterable", like how tooltip can be unregistered thanks to @chandlerprall's work in the PR you linked.

I think this leaves us with a few directions we can go in, and perhaps I'll leave it up to you & the team which is the best option:

  1. we can leave this line breaks plugin like it is now, keeping it the same as emoji: a default github-flavored markdown plugin that cannot be easily excluded
  2. we can make line breaks a special exception, and give it another exclude: ['line-breaks'] prop like how we invented exclude: ['tooltip']—although it might be a bit odd that we can't easily exclude other parsing plugins like emoji in this fashion
  3. we can make all default plugins "easily excluded", which will require minor refactors of each default plugin function, and document publicly all of the excludable plugins (I think there about 6? in total)
  4. some other option I haven't considered

let me know what you think! happy to help with whatever the team decides.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

These changes LGTM;

Since we target github flavor markdown, adding this feels more like a bug fix than what we'd consider a breaking change, and with that inclusion I'd consider this a non-disableable plugin, as opposed to our addition of tooltip to the markdown syntax (option 1 in #5272 (comment)).

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5272/

@i-a-n i-a-n marked this pull request as ready for review October 20, 2021 20:05
@i-a-n i-a-n merged commit cdf0ccd into elastic:master Oct 20, 2021
@i-a-n i-a-n deleted the markdown-plugin-breaks branch October 20, 2021 20:12
cchaos added a commit that referenced this pull request Oct 25, 2021
chandlerprall pushed a commit to chandlerprall/eui that referenced this pull request Oct 26, 2021
…c#5272)

* Add remark-breaks and line break plugin to MarkdownEditor

* Update changelog
chandlerprall pushed a commit to chandlerprall/eui that referenced this pull request Oct 26, 2021
ym pushed a commit to ym/eui that referenced this pull request Oct 29, 2021
…c#5272)

* Add remark-breaks and line break plugin to MarkdownEditor

* Update changelog
ym pushed a commit to ym/eui that referenced this pull request Oct 29, 2021
chandlerprall added a commit that referenced this pull request Nov 5, 2021
* Provide rows for datagrid cells to be owned by

* changelog

* undoing things

* working except virtualized container

* working row wrapper implementation

* Create datagrid row elements on-demand and render cells via portals

* Provide rows for datagrid cells to be owned by

* changelog

* undoing things

* working except virtualized container

* working row wrapper implementation

* Create datagrid row elements on-demand and render cells via portals

* Small style cleanup

* updated changelog

* fixing a bad changelog merge

* oh that style was important

* Update src/components/datagrid/body/data_grid_row_manager.ts

Co-authored-by: Constance <[email protected]>

* Update src/components/datagrid/data_grid_types.ts

Co-authored-by: Constance <[email protected]>

* Ensure unique ID in combobox with prepended or appended labels (#5229)

* Making an explicit check for ID prop in EUI Combobox

* Checking for a user-passed ID prop
* If no user-passed ID, we'll use the `rootId()` class method to set one
* Prepend and append props need an ID to relate label to the input

* Adding example for Combobox with prepended label
* Confirmed the label `for` attribute is referencing input `id`
* Added styles to round the edges of the child `<div>` when we prepend a
  label
* Bumped package.json

* Adding CHANGELOG entry and bumping version

* Update combo_box.tsx

* Updating styles to match other forms with prepend and apppend.

* Finishing Combobox CSS and test updates
* Simplified the Amsterdam overrides for prepended / appended labels
* Added conditional classes to `combo_box.tsx`
* Added unit tests for user passed ID, prepended and appended classes

* Reordering props in child component.

* Final adjustment to ID check in ComboBox

* Final updates to styling and documentation
* Added CSS rules for compressed, prepend and append border radius
* Updated the Display Toggles to include append
* Removed a duplicated instruction paragraph
* Simplified the logic for a unique ID in combobox.ts

* Adding Combobox unique ID to master changelog

* Updated Form Control Layout SCSS
* The borders were still rounded with prepend and append enabled
* Added two more rules to make borders square for full height and compressed

* Move to _combo_box.scss

Co-authored-by: cchaos <[email protected]>

* update i18ntokens

* 40.0.0

* Updated documentation.

* Add combined Jest+Cypress code coverage reports (#5262)

* Install Cypress code coverage dependencies

* Set up required babel+istanbul config

* Set up Cypress code coverage config

@see https://github.com/cypress-io/code-coverage#instrument-unit-tests

* Add script for combining cypress & jest code coverage
-into a single json & html report

* Misc cleanup/ignores

- Ignore new `.nyc_output` dir generated by cypress/istanbul

- Improve Jest config collectCoverageFrom !ignores:
  - DRY out component+services folder patterns with a {,} glob
  - Ignore new Cypress .spec files and .testenv files for jest coverage

- eslintignore the cypress folder, since they're all .js anyway and it has a decent amount of idiosyncracies
+ cleanup unused eslint-disable
+ cleanup cypress comment boilerplate
+ remove unused paths from webpack config

* Add wiki documentation

* [PR feedback] Move nyc config to own file

* [PR feedback] Switch combine coverage script to .sh

* [EuiMarkdownEditor] Add `remark-breaks` and line break plugin (#5272)

* Add remark-breaks and line break plugin to MarkdownEditor

* Update changelog

* [Docs] Separated out Borders to its own page & [EuiTableRowCell] fixes (#5283)

* Moved Border tokens to its own page
* Figured out a way to get subsections in the sidenav working
* Added the new `_values` under `customizing/`

* [EuiTableRowCell] Actually support `valign` property manually
* [EuiTable*] Fixing some props and mobile options
* Slightly better responsive table styles

* Fix CL from #5272

* [Docs] Update EuiDatePicker types (#5318)

* omit unsupported types

* resolve console errors

* CL

* [Cypress] Add flakey test retries + harden intermittent context menu failures (#5317)

* Add Cypress failed test retries to CI run mode

* Increase cy.wait on flaky context menu tests (just in case)

* More hardening

* [Docs] Fix Colors guidelines (#5316)

* use useJsonVars

* use variable

* Update dependency @elastic/charts to ^38.1.0 (#5321)

* PR feedback

* Added a cypress test for datagrid row rendering

* Revert changes(?) to docs/

* revert changelog reformats

* re-ignore a couple datagrid example pages when running a11y tests

* Alternate way to know if the datagrid cells have rendered

* PR feedback

* changelog

Co-authored-by: Constance <[email protected]>
Co-authored-by: Trevor Pierce <[email protected]>
Co-authored-by: cchaos <[email protected]>
Co-authored-by: ian moersen <[email protected]>
Co-authored-by: Caroline Horn <[email protected]>
Co-authored-by: Greg Thompson <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

[EuiMarkdownEditor] Enhancement request: "Newlines -> line breaks" as a default plugin
4 participants