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

SVG borders: Respond to dynamic changes in selection border col… #6593

Merged

Conversation

warpech
Copy link
Member

@warpech warpech commented Jan 2, 2020

Context

This PR is a response to the comment #6467 (comment). It the selection borders, selection corner (desktop) and selection handles (mobile) to respond to the dynamic changes of width and color setting.

For example, use the below code in Handsontable to set border of the currently selected cell to be "1px green":

hot.selection.highlight.cell.settings.border.width = 1;
hot.selection.highlight.cell.settings.border.color = 'green';
hot.render();

Default selection settings:

hot.selection.highlight.cell.settings.border.width = 2;
hot.selection.highlight.cell.settings.border.color = '#4b89ff';
hot.selection.highlight.fill.settings.border.width = 1;
hot.selection.highlight.fill.settings.border.color = '#ff0000';
hot.selection.highlight.areas.forEach((selection) => {
  selection.settings.border.width = 1;
  selection.settings.border.color = '#4b89ff';
});
hot.render();

☢️ However, there is one serious pitfall: selection.highlight.areas objects are instantiated only after they are used for the first time. So you can only set them after the end user made an area selection (multiple area selections) for the first time.

How has this been tested?

I added an automated test for desktop and mobile.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. Rewrite custom borders to use SVGs insead of DIVs #6467 (comment)
  2. Allow to change styling for selected areas by introducing currentSelectionClassName  #6538
  3. Changing rectangle cursor color for different cells #2487

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation.

This commit allows the selection borders, selection corner (desktop) and selection handles (mobile) to respond to the dynamic changes of width and color setting.

For example, use the below code in Handsontable to set border of the currently selected cell to be "1px green":

```
hot.selection.highlight.cell.settings.border.width = 1;
hot.selection.highlight.cell.settings.border.color = 'green';
hot.render();
```

Default selection settings:

```
hot.selection.highlight.cell.settings.border.width = 2;
hot.selection.highlight.cell.settings.border.color = '#4b89ff';
hot.selection.highlight.fill.settings.border.width = 1;
hot.selection.highlight.fill.settings.border.color = '#ff0000';
hot.selection.highlight.areas.forEach((selection) => {
  selection.settings.border.width = 1;
  selection.settings.border.color = '#4b89ff';
});
hot.render();
```
@warpech warpech changed the title change: make selection border color and width respond to dynamic changes SVG borders: Respond to dynamic changes in selection border color and width Jan 2, 2020
@aninde
Copy link
Contributor

aninde commented Jan 8, 2020

I didn't find regression and other bugs, but I had a problem reproducing a new feature. With the help of @warpech I was able to write it in the function and only in this way it worked in my environment. I need to debug those to make a demo. Right now I can show on gif working of selection.highlight.areas

function changeSelection() {
        hot.selection.highlight.cell.settings.border.width = 2;
        hot.selection.highlight.cell.settings.border.color = 'red';
        hot.selection.highlight.fill.settings.border.width = 2;
        hot.selection.highlight.fill.settings.border.color = 'pink';

        hot.selection.highlight.cell.settings.border.cornerVisible = false; //example from #6538
        hot.selection.highlight.cell.settings.border.lineStyle = true; //example from #6538

        hot.selection.highlight.areas.forEach((selection) => {
            selection.settings.border.width = 2;
            selection.settings.border.color = 'green';
        });
        hot.render()
    }
    window.changeSelection = changeSelection;
    window.hot = hot;

changeSelection-ff-

You can change style of selection.areas only for number if instances that were created. On the gif, changed style have only two instances, the thirt one has default style.

previously the inset value of the border was fixed to 1, making it look right only when the border width was 2. Now the inset is calculated correctly from any border width.
@aninde aninde added this to the December 2019 milestone Jan 9, 2020
@aninde
Copy link
Contributor

aninde commented Jan 9, 2020

New issue explaining a problem discovered in this PR: #6627 (colors order on fixed borders)

@aninde
Copy link
Contributor

aninde commented Jan 10, 2020

New issue explaining a problem discovered in this PR: #6628 (selection, fill and autoFill covers fixedBorder or frozen-border)

@warpech warpech force-pushed the feature/issue-6064-dynamic-border-settings branch from 877d933 to 3e13b24 Compare January 10, 2020 22:38
…the border of a selection is rendered

previously, the className "current" had a magical hard-coded consequence of changing the border to be rendered inside of the cell.

From now on, this behavior is defined by setting a property `strokeAlignment: 'inside'` on the border settings.

The name `strokeAlignment` is inspired by the new SVG Strokes spec proposal: https://svgwg.org/specs/strokes/#SpecifyingStrokeAlignment
…ight edges

according to the intended rendering of cell selections, all 4 borders should be rendered inside of the cell.
With the previous implementation that worked well only with border width 2px.
After this change, it works like intended with any border width.
… cell

this is not a immediately noticeable change, because it renders the same as they have 1px width.
However, when a customer uses a different configuration, they are supposed to be rendered inside of the cell and it will become visible.
@warpech
Copy link
Member Author

warpech commented Jan 13, 2020

As discussed on the today's status meeting, I will merge this PR without review to the "master" PR #6157, because having a single source-of-truth PR is easier both for code reviewer and the tester.

@warpech warpech changed the title SVG borders: Respond to dynamic changes in selection border color and width SVG borders: Respond to dynamic changes in selection border col… Jan 13, 2020
@warpech warpech merged commit 5f020dd into feature/issue-6064 Jan 13, 2020
@warpech warpech deleted the feature/issue-6064-dynamic-border-settings branch January 13, 2020 10:38
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.

None yet

2 participants