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

Data faceting #538

Open
wants to merge 13 commits into
base: kb-MENG
Choose a base branch
from
Open

Data faceting #538

wants to merge 13 commits into from

Conversation

ktbacher
Copy link
Contributor

No description provided.

@ktbacher ktbacher added the 🚧 WIP Work in Progress PRs label Apr 16, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 16, 2021

This pull request introduces 1 alert when merging 1949056 into 553c019 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@ktbacher ktbacher removed the 🚧 WIP Work in Progress PRs label May 5, 2021
import {State} from '../store';
import {Dispatch} from 'redux';

export function addFacetLayout (payload: FacetLayoutRecord) {
Copy link
Member

Choose a reason for hiding this comment

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

re: this file, facetLayoutReducer, and facetLayout.ts, i think they're related enough to core layout functionality that i would consider just adding the facet actions / reducer cases / record definitions to the existing layout files so that it's easier to find everything.

/**
* Layouts align multiple groups
*/
export interface FacetLayout {
Copy link
Member

Choose a reason for hiding this comment

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

is there enough overlap between FacetLayout and Layout that we can think about e.g.:

  • a common interface that both of them "inherit" from via the typescript & (intersection type) operator
  • a "parent type" defined as the | (union type) of the two?

i'm not suggesting either particular choice is more right here but consider whether or not that might simplify things in some places (it also might not simplify things at all, in which case feel free to reject this idea)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, they are similar in concept but very different in implementation and how they are used in Lyra and the ultimate vega spec. Perhaps worth discussing further

src/js/ctrl/export.ts Outdated Show resolved Hide resolved
const layout = clean(duplicate(facetLayouts), internal);
const id = Object.keys(layout)[Object.keys(layout).length -1];
return layout[id];
}
Copy link
Member

Choose a reason for hiding this comment

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

if i'm understanding this correctly, the logic is:

  • if there are any facet layouts in the lyra store, set the last one created as the layout in the exported spec

a few questions to help my understanding:

  • why do we discard the previous ones?
  • what are the cases where there will be more than one facet layout?
  • how is this different from how non-facet layouts are handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really wasn't sure on how the logic of the export worked so this might be something we can walk through tomorrow

@jonathanzong
Copy link
Member

awesome work! i left some comments, questions, and suggestions. exiting to see this getting so close!

@@ -52,6 +54,7 @@ const getDefaultState = Record<LyraState>({
widgets: Map<string, WidgetRecord>(),
signals: defaultSignalState,
layouts: Map<string, LayoutRecord>(),
facetLayouts: Map<string, FacetLayoutRecord>(),
Copy link
Member

Choose a reason for hiding this comment

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

suggestion per our conversation today: since there can only be one vega layout spec, change this from a Map to a single FacetLayoutRecord

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably just not understanding typescript syntax/types here, but when I change the line to facetLayouts: FacetLayoutRecord I get an error:'FacetLayoutRecord' only refers to a type, but is being used as a value here.

Copy link
Member

Choose a reason for hiding this comment

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

Here you're initializing the actual values of a record, not defining an interface. So what you're trying to do in this line is set the value of a property facetLayouts to a default FacetLayoutRecord. So it should be facetLayouts: FacetLayoutRecord() -- note the parentheses for initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I'm getting the same error even with the parentheses

Copy link
Member

Choose a reason for hiding this comment

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

i meant FacetLayout(), for the constructor and not the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh make sense. realizing with the proposed change to facet layouts to make it compatible with the other layout mechanism, we could in theory support multiple facets and might want to keep it as a map after all?

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.

2 participants