Skip to content

SCSS and Legend Panel #54

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

Merged
merged 15 commits into from
Nov 13, 2017
Merged

Conversation

bpostlethwaite
Copy link
Member

@bpostlethwaite bpostlethwaite commented Nov 10, 2017

closes #23 #24 and #27

Although this PR closes multiple legend items they are a fairly straightforward product of all the SCSS handling commits. We start migrating workspace scss over to react-plotly.js-editor. SCSS folder structure and naming schemes are altered to comply with BEM conventions and match the component hierarchy in the JS source.

A bunch of widgets are deleted. We will only migrate what is strictly necessary.

Colorpicker and SubPanel cogmenu features are implemented on top of migrated SCSS.

@bpostlethwaite
Copy link
Member Author

image

@bpostlethwaite
Copy link
Member Author

bpostlethwaite commented Nov 10, 2017

The API is interesting and worth discussing:

            <Section name={_('Positioning')}>
              <SubPanel>
                <Section name={_('Anchor Point')}>
                  <Info>
                    {_(
                      'The positioning inputs are relative to the ' +
                        'anchor points on the text box'
                    )}
                  </Info>
                  <Radio
                    attr="legend.xanchor"
                    options={[
                      {label: _('Left'), value: 'left'},
                      {label: _('Center'), value: 'center'},
                      {label: _('Right'), value: 'right'},
                    ]}
                  />
                </Section>
              </SubPanel>
              <Numeric
                label={_('X Position')}
                step={0.01}
                attr="legend.x"
                postfix="px"
              />
            </Section>
            <Section name={_('Trace Order')}>
              <Radio
                attr="legend.traceorder"
                options={[
                  {label: _('Normal'), value: 'normal'},
                  {label: _('Reversed'), value: 'reversed'},
                ]}
              />
            </Section>

You declare a cogmenu by specifying a SubPanel as any direct child of a Section. Section will elevate that child to the cogmenu position. Nesting SubPanels works as expected.

@bpostlethwaite
Copy link
Member Author

bpostlethwaite commented Nov 13, 2017

Caveat: A Section with a cogmenu will always be visible even if all of its children are hidden and the cogmenu is empty. The more thorough way to avoid this problem would be to somehow query the content of the CogMenu and check its visibility but there isn't an easy way to do that...

This same problem will occur with <Panel> visibility. Right now we have <Field> and <Section> level show and hide behaviour only.

@alexcjohnson
Copy link
Collaborator

<SubPanel> seems a bit ambiguous to me - could be something that's always visible but just set apart somehow... your comment calls it a cogmenu, what about solidifying that to <CogMenu>? One more question: what happens if you declare two cogmenus in one <Section>? I suppose we could imagine uses for that, if each menu could have a distinct icon (in which case <CogMenu> would cease to be a good name... hmm)

@bpostlethwaite
Copy link
Member Author

Right yes, and the icon is configurable so in some cases even if there is one menu it may not be a cog.

@bpostlethwaite
Copy link
Member Author

<Settings> ?

@alexcjohnson
Copy link
Collaborator

<MenuPanel>? I want something that clearly indicates that it's normally hidden.

@bpostlethwaite
Copy link
Member Author

<ClickMenu> or <ClickPanel> <OptionPanel> <MenuPanel>... dunno. Easy to change at any point regardless.

@bpostlethwaite
Copy link
Member Author

(well until we release ;) )

@bpostlethwaite
Copy link
Member Author

Also <Fold> is probably in need of some thought.

@@ -0,0 +1,59 @@
.field {
Align-items: center;
Copy link
Collaborator

Choose a reason for hiding this comment

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

lowercase?

Copy link
Member Author

@bpostlethwaite bpostlethwaite Nov 13, 2017

Choose a reason for hiding this comment

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

yes good catch. Edited in my current Axes PR branch.

@@ -47,6 +47,24 @@ describe('Section', () => {
).toBe(false);
});

fit('is visible if it contains any non attr children', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this behavior? Seems kinda like you wouldn't want <Info> to be enough to invoke display, or maybe sometimes you do but not always?

⚠️ committed fit ⚠️ (you probably fixed this later... but at least in plotly.js we have a syntax test checking for fit et al in all test files)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep I did catch that one. And yep we have the same test in the workspace. I'll make an issue for that right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

#57

Hmmm yeh I could see skipping <Info> actually. But not <MenuPanel>? Unless you interrogate the <Section>s inside <MenuPanel> ... but that would take some serious gymnastics...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right, that's your caveat above. It might be worth thinking a bit about whether there really are any cases where only the <MenuPanel> exists to make a <Section> visible. If that's really "advanced settings" we might be able to argue that if none of the basics for that section exist, there's never a case where the advanced would be needed?

I'm not sure if any similar argument works for <Panel>. Would be worthwhile looking for a concrete example.

Copy link
Member Author

Choose a reason for hiding this comment

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

True I'll add this to an issue and deal with it once I have some breathing room

Copy link
Member Author

Choose a reason for hiding this comment

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

#58

@alexcjohnson
Copy link
Collaborator

Love the massive 🔪 in this PR 🎉
Looks like you've addressed all my comments either in issues or other WIP, so 💃 !

@bpostlethwaite
Copy link
Member Author

Awesome thanks again for the reviews! I'll topple these dominoes tomorrow and continue working on Axes as that is the major feature hurdle/deadline. Once that is complete I'll revisit many of the suggestions you have brought forth.

@bpostlethwaite bpostlethwaite merged commit 9367381 into CenteringOfFieldsWithNoLabels Nov 13, 2017
@bpostlethwaite bpostlethwaite deleted the SubMenus branch November 13, 2017 14:47
This was referenced Nov 15, 2017
@bpostlethwaite bpostlethwaite changed the title Sub menus and Colorpicker SCSS and Legend Panel Nov 15, 2017
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