Skip to content

Conversation

@michaelmang
Copy link
Contributor

Tooltip

Screen Shot 2020-04-03 at 1 45 46 PM

Select

Screen Shot 2020-04-03 at 1 58 23 PM

Screen Shot 2020-04-03 at 1 58 36 PM

✅ Required and Optional variants

@netlify
Copy link

netlify bot commented Apr 3, 2020

Deploy preview for helix-react ready!

Built with commit fd7b466

https://deploy-preview-85--helix-react.netlify.app

Copy link
Contributor

@nicko-winner nicko-winner left a comment

Choose a reason for hiding this comment

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

Thanks! A couple nits and questions...Storybook in Dark mode looks nice.

<Demo required />
);
})
.add('Optional', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Other components use knobs for configuration. One of the advantages is they allow for multiple kinds of configuration at the same time such as "required, and disabled"
2020-04-03_15-34-08

Copy link
Contributor

@nicko-winner nicko-winner Apr 3, 2020

Choose a reason for hiding this comment

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

my "required, disabled" example might not have been the best demonstration of the point I was trying to make. But the knobs do offer a consistent approach to manipulating the components and can be themed.

Copy link
Contributor

@nicko-winner nicko-winner Apr 3, 2020

Choose a reason for hiding this comment

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

At the heart of this comment is just a desire to have an approach that's consistent for changing component state across the project. If we use use knobs, great! if we controls inside the canvas like the tool tip story, I can get behind that too. Just looking for a consistent approach, we can apply so all stories work in a similar way. What are your thoughts? If there are no strong opinions yet, we can deal with it latter when there is more of a knowing about which approaches seem to work better, might just mean a little more rework if we wait too long.

Copy link
Member

Choose a reason for hiding this comment

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

@HelixDesignSystem/helix-react-core, I agree it's better to go with @nicko-winner's recommendation to have a consistent approach:

At the heart of this comment is just a desire to have an approach that's consistent for changing component state across the project. If we use use knobs, great! if we controls inside the canvas like the tool tip story, I can get behind that too. Just looking for a consistent approach, we can apply so all stories work in a similar way. What are your thoughts? If there are no strong opinions yet, we can deal with it latter when there is more of a knowing about which approaches seem to work better, might just mean a little more rework if we wait too long.

I defer to you all for the LOE between using knobs vs another approach, though it does seem like knobs provide (implementation) consistency and flexibility.

Copy link
Member

@100stacks 100stacks left a comment

Choose a reason for hiding this comment

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

@HelixDesignSystem/helix-react-core great work and discussions guys...we're making progress! 💯

<Demo required />
);
})
.add('Optional', () => {
Copy link
Member

Choose a reason for hiding this comment

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

@HelixDesignSystem/helix-react-core, I agree it's better to go with @nicko-winner's recommendation to have a consistent approach:

At the heart of this comment is just a desire to have an approach that's consistent for changing component state across the project. If we use use knobs, great! if we controls inside the canvas like the tool tip story, I can get behind that too. Just looking for a consistent approach, we can apply so all stories work in a similar way. What are your thoughts? If there are no strong opinions yet, we can deal with it latter when there is more of a knowing about which approaches seem to work better, might just mean a little more rework if we wait too long.

I defer to you all for the LOE between using knobs vs another approach, though it does seem like knobs provide (implementation) consistency and flexibility.

@michaelmang
Copy link
Contributor Author

Prettier

Screen Shot 2020-04-13 at 3 37 46 PM

I created a prettier.config.js file that defines all the settings of prettier that we want to explicitly call out.

When you run npm run prettify, it will automatically format all code against the prettier settings.

Every time you do git commit, the prettify script will run. In theory, you will then be able to put together a commit for your "prettified" code after this executes.

Our teams moves this into the webpack bundle pipeline so it becomes automatic but I'm deferring that here.

If you add a prettier plugin to your code editor, that will also format against our prettier settings.

I'm open to suggestions!

@michaelmang
Copy link
Contributor Author

@nicko-winner @100stacks this is ready for re-review

Copy link
Contributor

@nicko-winner nicko-winner left a comment

Choose a reason for hiding this comment

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

LGTM, but just noticed tooltip is missing some of the attributes / events. Can handle in a followup PR or address now.


const Tooltip = ({ children, id, position }) => {
return (
<hx-tooltip for={id} position={position}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the events and attributes listed on this page: https://helixdesignsystem.github.io/helix-ui/elements/hx-tooltip/ ?

Copy link
Member

@100stacks 100stacks left a comment

Choose a reason for hiding this comment

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

Dev LGTM

@100stacks
Copy link
Member

Fixes Issue #31 (<Tooltip/>) and Issue #87 (<Select/>).

@100stacks 100stacks merged commit 33c8c84 into master Apr 16, 2020
@100stacks 100stacks deleted the select-tooltip branch April 16, 2020 22:00
@100stacks 100stacks added this to the v1.0.0-rc.0 milestone Jul 21, 2020
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.

4 participants