[Feature] Multiple toolbox items for single tool #2050
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* FIx mobile popover fixed positioning * Add mobile popover overlay * Hide mobile popover on scroll * Alter toolbox buttons hover * Fix closing popover on overlay click * Tests fix * Fix onchange test * restore focus after toolbox closing by ESC * don't move toolbar by block-hover on mobile Resolves #1972 * popover mobile styles improved * Cleanup * Remove scroll event listener * Lock scroll on mobile * don't show shortcuts in mobile popover * Change data attr name * Remove unused styles * Remove unused listeners * disable hover on mobile popover * Scroll fix * Lint * Revert "Scroll fix" This reverts commit 82deae5. * Return back background color for active state of toolbox buttons Co-authored-by: Peter Savchenko <[email protected]>
* Replace visibility property with display for hiding popover * Disable arrow right and left keys for popover * Revert "Replace visibility property with display for hiding popover" This reverts commit af521cf. * Hide popover via setting max-height to 0 to fix animation in safari * Remove redundant condition
* Change popover opening direction based on available space below it * Update check * Use cacheable decorator
Co-authored-by: George Berezhnoy <[email protected]>
…/editor.js into feat/vertical-toolbox
….com/codex-team/editor.js into feat/vertical-toolbox-multiple-items
neSpecc
approved these changes
May 30, 2022
gohabereg
approved these changes
May 30, 2022
| * extend converted data with this item's "data" overrides | ||
| */ | ||
| if (blockDataOverrides) { | ||
| newBlockData = Object.assign(newBlockData, blockDataOverrides); |
There was a problem hiding this comment.
There might be a problem if override is not on the first level.
Eg.
// data
{
settings: {
level: 1,
anotherProp: 'value',
}
}
// override
{
config: {
level: 2
}
}
Maybe just left a todo to use deep merge in the future
There was a problem hiding this comment.
We will add a note at the docs describing that data overrides should have the same structure that block data
| /** | ||
| * Skip tools without «import» rule specified | ||
| */ | ||
| if (!conversionConfig || !conversionConfig.import) { | ||
| return; | ||
| } | ||
|
|
||
| this.addTool(name, toolboxSettings.icon, toolboxSettings.title); | ||
| if (Array.isArray(tool.toolbox)) { |
There was a problem hiding this comment.
Maybe we can unify that to array somewhere at the start, so we don't need to add checks everywhere
neSpecc
reviewed
Jun 16, 2022
neSpecc
reviewed
Jun 17, 2022
src/components/ui/toolbox.ts
Outdated
| /** | ||
| * Merge real tool's data with data overrides | ||
| */ | ||
| const defaultBlockData = await this.api.blocks.composeBlockData(toolName); |
There was a problem hiding this comment.
I think we should do that only if we have the blockDataOverrides. In other cases (insertion of a regular tool) we will skip extra block construction/saving/merging.
Co-authored-by: Peter Savchenko <[email protected]>
Co-authored-by: Peter Savchenko <[email protected]>
neSpecc
approved these changes
Jun 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support of multiple toolbox entries for single tool.
A tool's toolbox property now can be represented as an array of items. Which item can include tool's default config overrides (such as level for header tool). These config overrides are merged with default tool settings when inserting relevant block.
Example of tool with multiple toolbox items can be found at editor-js/header#79