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

[lab] Create lab package, add SpeedDial #10288

Merged
merged 2 commits into from
Feb 19, 2018
Merged

[lab] Create lab package, add SpeedDial #10288

merged 2 commits into from
Feb 19, 2018

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Feb 13, 2018

No description provided.

@oliviertassinari oliviertassinari added the package: lab Specific to @mui/lab label Feb 15, 2018
@mbrookes mbrookes changed the title [WIP][lab] Create lab package, add SpeedDial [lab] Create lab package, add SpeedDial Feb 17, 2018
@mbrookes mbrookes added the component: speed dial This is the name of the generic UI component, not the React module! label Feb 17, 2018
@@ -64,10 +77,6 @@ function buildDocs(options) {
reactAPI.pagesMarkdown = pagesMarkdown;
reactAPI.src = src;

// if (reactAPI.name !== 'Backdrop') {
Copy link
Member

@oliviertassinari oliviertassinari Feb 17, 2018

Choose a reason for hiding this comment

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

This comment is very useful for debugging, more than useful.

Copy link
Member Author

@mbrookes mbrookes Feb 17, 2018

Choose a reason for hiding this comment

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

Comment? Looks like commented out dead code. Perhaps the "comment" needs a comment. 😄

@@ -21,7 +21,7 @@ export function pageToTitle(page) {

const name = page.pathname.replace(/.*\//, '');

if (page.pathname.indexOf('/api') === 0) {
if (page.pathname.indexOf('/api') !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

This is less restrictive, why?

Copy link
Member Author

@mbrookes mbrookes Feb 17, 2018

Choose a reason for hiding this comment

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

To pick up /lab/api, but I can make it more verbose.

@@ -32,7 +32,7 @@ to discuss the approach before submitting a PR.
- **[Buttons: Floating Action Button](https://material.io/guidelines/components/buttons-floating-action-button.html) ✓**
- **[Trigger](https://material.io/guidelines/components/buttons-floating-action-button.html#buttons-floating-action-button-transitions) ✓**
- [Toolbar](https://material.io/guidelines/components/buttons-floating-action-button.html#buttons-floating-action-button-transitions)
- [Speed dial](https://material.io/guidelines/components/buttons-floating-action-button.html#buttons-floating-action-button-transitions)
- **[Speed dial](https://material.io/guidelines/components/buttons-floating-action-button.html#buttons-floating-action-button-transitions) ✓**
Copy link
Member

Choose a reason for hiding this comment

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

Is a component in the lab makes it supported?
It doesn't even mean it will reach core. We might decide to kill one lab component for some reason.

Copy link
Member Author

@mbrookes mbrookes Feb 17, 2018

Choose a reason for hiding this comment

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

Is a component in the lab makes it supported?

Legacy change - don't forget SpeedDial was written before Lab.

We might decide to kill one lab component for some reason.

Sure. But not this one. 😉

@@ -0,0 +1 @@
../../.gitignore
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this file?

Copy link
Member Author

@mbrookes mbrookes Feb 17, 2018

Choose a reason for hiding this comment

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

Apparently not! (Can't repro whatever might have inspired having a package-local .gitignore before...)

@@ -0,0 +1 @@
Note: Not active, just a placeholder for the SpeedDial regression test.
Copy link
Member

Choose a reason for hiding this comment

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

Should the regression tests be shared between different packages?

Copy link
Member Author

Choose a reason for hiding this comment

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

SpeedDial was originally written to go into material-ui/src, so a regression test was written. Rather than throw that code away, I've put it here. Nothing more, nothing less.

modal: 1300,
snackbar: 1400,
tooltip: 1500,
speedDial: 1100,
Copy link
Member

@oliviertassinari oliviertassinari Feb 17, 2018

Choose a reason for hiding this comment

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

This is replicated in the .md, to update.
It's a breaking change and a dangerous one as can have large impact without people noticing.

@mui mui deleted a comment from mbrookes Feb 17, 2018
@oliviertassinari oliviertassinari self-assigned this Feb 17, 2018
@oliviertassinari
Copy link
Member

I'm making the lab/core packages my priority for this weekend. Let's scale :).

@@ -23,7 +23,7 @@ function findPagesMarkdown(
}
let pathname = itemPath.replace(new RegExp(`\\${path.sep}`, 'g'), '/').replace(/^.*\/pages/, '').replace('.md', '');

if (pathname.indexOf('/demos') === 0) {
if (pathname.indexOf('/demos') === 0 || pathname.indexOf('/lab') === 0 || pathname.indexOf('/utils') === 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

I will try to find a better approach than having to explicit the folders that follow a different structure in order to fix #10337.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: speed dial This is the name of the generic UI component, not the React module! package: lab Specific to @mui/lab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants