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

Jump to bottom

[Grouping] improvements #27581

Merged
merged 6 commits into from
Dec 27, 2018
Merged

[Grouping] improvements #27581

merged 6 commits into from
Dec 27, 2018

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Dec 20, 2018 •
edited
Loading

Improvement to grouping:

@monfera monfera requested a review from a team as a code owner December 20, 2018 14:34
@monfera monfera changed the title chore: remove asynchronous element selection [Grouping] improvements Dec 20, 2018
@monfera monfera self-assigned this Dec 20, 2018
@monfera monfera added review chore Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Dec 20, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@monfera monfera force-pushed the grouping-improvement branch from dba0e52 to 023c2dd Compare December 20, 2018 16:03
@elastic elastic deleted a comment from elasticmachine Dec 20, 2018
@elastic elastic deleted a comment from elasticmachine Dec 20, 2018
@elastic elastic deleted a comment from elasticmachine Dec 20, 2018
@monfera monfera force-pushed the grouping-improvement branch from c40abaf to 9afa0a1 Compare December 20, 2018 22:03
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine Dec 20, 2018
@w33ble w33ble self-requested a review December 20, 2018 23:19
@monfera monfera force-pushed the grouping-improvement branch from 9afa0a1 to d9afefc Compare December 20, 2018 23:25
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@monfera monfera mentioned this pull request Dec 21, 2018
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

w33ble
w33ble reviewed Dec 26, 2018
View reviewed changes
x-pack/plugins/canvas/public/state/reducers/elements.js
workpadState,
['pages', pageIndex, getLocation(element.position.type)],
trimElement(element)
return elements.reduce(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: while I appreciate this bit, building an object and using assign might be easier to comprehend.

w33ble
w33ble reviewed Dec 26, 2018
View reviewed changes
x-pack/plugins/canvas/public/state/reducers/elements.js
@@ -10,6 +10,7 @@ import { get } from 'lodash';
import * as actions from '../actions/elements';

const getLocation = type => (type === 'group' ? 'groups' : 'elements');
const firstOccurrence = (element, index, array) => array.indexOf(element) === index;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a neat little de-duping helper, so rare to see that third arg used. 👍

nit: Since this is only used in 1 place, keeping it near where it's used is probably better for comprehension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @w33ble! IIRC there are a few other places in the layout engine that use such a function except they're inlined; if that's the case, I'd move it to functional.js and link to it from wherever else it's repeated now, otherwise move it near where it's used. Good you mentioned this rule of thumb, because otherwise I tended to put small, generic functions atop of files even if they were used only once.

@w33ble w33ble requested a review from cqliu1 December 26, 2018 20:38
w33ble
w33ble approved these changes Dec 26, 2018
View reviewed changes
Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

LGTM, your call if you want to address either of my nits or not

cqliu1
cqliu1 approved these changes Dec 26, 2018
View reviewed changes
Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for fixing the duplicate page issue on old workpads. I just had one nitpick, but it's not a blocker.

x-pack/plugins/canvas/public/state/actions/elements.js
({ dispatch, type }, element, pageId) => {
const newElement = { ...getDefaultElement(), ...getBareElement(element) };
// move the element so users can see that it was added
export const insertNodes = createThunk('insertNodes', ({ dispatch, type }, elements, pageId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This function looks awfully similar to the addElement function. Can we just delete addElement and maybe rename this addElements since it handles adding multiple elements?

The only difference is this function adds an offset to the position of the new element, but I feel like the offset could be added elsewhere. It'd be a nice refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cqliu1 good point! I'll handle it as a separate small PR, as would I do with @w33ble's suggestions.

@monfera monfera merged commit 0113e5a into master Dec 27, 2018
This was referenced Dec 27, 2018
Merged
Merged
monfera added a commit to monfera/kibana that referenced this pull request Dec 27, 2018
@monfera
[Grouping] improvements (elastic#27581)
1a05ea5 
* chore: remove asynchronous element selection

* chore: remove duplication between duplicateElement and rawDuplicateElement

* chore: renamed rawDuplicateElement to insertNodes (pure rename, not yet set based)

* perf: bulk node insertion

* chore: removed unnecessary element id duplications

* fix: supply empty array if preexisting workbook had none
@monfera monfera added the v6.7.0 label Dec 27, 2018
monfera added a commit to monfera/kibana that referenced this pull request Dec 27, 2018
@monfera
[Grouping] improvements (elastic#27581)
26e4b2b 
* chore: remove asynchronous element selection

* chore: remove duplication between duplicateElement and rawDuplicateElement

* chore: renamed rawDuplicateElement to insertNodes (pure rename, not yet set based)

* perf: bulk node insertion

* chore: removed unnecessary element id duplications

* fix: supply empty array if preexisting workbook had none
monfera added a commit that referenced this pull request Dec 27, 2018
@monfera
[Grouping] improvements (#27581) (#27820)
d211a6b 
* chore: remove asynchronous element selection

* chore: remove duplication between duplicateElement and rawDuplicateElement

* chore: renamed rawDuplicateElement to insertNodes (pure rename, not yet set based)

* perf: bulk node insertion

* chore: removed unnecessary element id duplications

* fix: supply empty array if preexisting workbook had none
monfera added a commit that referenced this pull request Dec 27, 2018
@monfera
[Grouping] improvements (#27581) (#27819)
9345980 
* chore: remove asynchronous element selection

* chore: remove duplication between duplicateElement and rawDuplicateElement

* chore: renamed rawDuplicateElement to insertNodes (pure rename, not yet set based)

* perf: bulk node insertion

* chore: removed unnecessary element id duplications

* fix: supply empty array if preexisting workbook had none
@monfera monfera deleted the grouping-improvement branch December 27, 2018 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cqliu1 cqliu1 cqliu1 approved these changes

@w33ble w33ble w33ble approved these changes

Assignees

@monfera monfera

Labels
chore review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.6.0 v6.7.0 v7.0.0
Projects
None yet
Milestone
No milestone
4 participants
@monfera @elasticmachine @w33ble @cqliu1