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

[Core] reorg internal modules #3682

Closed
wants to merge 1 commit into from
Closed

[Core] reorg internal modules #3682

wants to merge 1 commit into from

Conversation

mbrookes
Copy link
Member

This was my take on reorganising the internal modules. Pretty much everything is done, apart from styles, which I've mostly left alone for now until we decide our approach to themes in #3678.

  1. Rename src/utils to /src/common & delete index.js.
  2. Move src/mixins/* to src/common/ (these are going, but no need to keep the directory around).
  3. Move and CamelCase internal shared components to src/common/components/.*
  4. Move src/InkBar.jsx to src/tabs/InkBar.jsx.
  5. Move src/enhanced-texarea.jsx to src/TextField/TextFieldTextarea.jsx.
  6. Move /src/styles/getMuiTheme.js to src/getMuiTheme.js & deprecate old export path.
  7. Copy src/styles/colors.js to src/styles/colors.js Exports in the old colors.js aren't deprecated yet. (Hoping there's a low overhead way of doing that!)
  8. Clean up src/index.js to remove non-public exports.
  9. Update imports in src.
  10. Update imports in docs.
  11. Update imports in tests.

*While common components are currently grouped in src/common/components, this can be flattened into src/common/ if preferred, or could be a sibling directory in /src.

This hopefully covers most of #3670 (pending a decision on the final name for internal - Edit: I've used common for now.).

@alitaheri - sorry this doesn't directly continue from your good work, but we both started on this in parallel. I guess we were both keen to see this happen ASAP! 😁 (And I updated this to benefit from the discussion there.)

@callemall/material-ui - hope you like! 😅

@alitaheri
Copy link
Member

@mbrookes Wow awesome work 😍 😍 Thanks a lot 👍 👍

@chrismcv
Copy link
Contributor

This looks good to me. And I'm happy with common.

@nathanmarks
Copy link
Member

I don't like TextField/TextFieldTextarea.

The capitalization looks weird, and the name of the component is weird. Using that logic, we should also have a TextFieldInput, or a TableRowTableColumn.

Additionally, this has brought up an interesting point RE api design amongst components with variations such as this.

Let's look at button as an example. Rather than having a <Button /> with an optional raised prop, we have 2 separate button components, FlatButton and RaisedButton. But with TextField, we have an optional multiline prop that creates a textarea, rather than a TextField and MultilineTextField.

I'm not arguing one way or the other here -- just presenting the thought.

@chrismcv
Copy link
Contributor

My hope is that we'll end up with TextField and a MultiLine text field component once we sort the compose-ability of TextField in 0.16?

I think different components should be used for different behaviour, rather than for different formatting, and FlatButton seems like a styling concern...? I quite like <TextField.Multiline /> for "scoped" behaviour segmentation.

@nathanmarks
Copy link
Member

@chrismcv I'm on the same page with regards to styling concerns vs behaviour differences.

I didn't know you could "scope" behaviour like that (with a period), how is it implemented?

Also, with regards to the current name EnhancedTextarea, or the lovely proposed name, TextFieldTextarea 😄 . I may be wrong, but I feel as though if HTML was PascalCase, it would be TextArea if they also had a TextField component since they are composed of the same parts of speech.

@mbrookes
Copy link
Member Author

While we're deciding what to rename EnhancedTextArea, I have one other niggle.

Having components down inside common doesn't seem right. What about moving it up as src/commonComponents, or even just src/components, which would leave src/common free to just be src/utils. 😄

@chrismcv
Copy link
Contributor

@nathanmarks - I think the following should work. We use it es5 style atm.

export default TextField;`
export {Multiline};

@nathanmarks
Copy link
Member

@chrismcv Eeek, I don't like mixing es6 import/export syntax and es5 usage like that.

@nathanmarks
Copy link
Member

@mbrookes Yeah, I disliked seeing this:

TextField/EnhancedTextarea
common/components/EnhancedSwitch

@nathanmarks
Copy link
Member

@chrismcv I think that a decision should be made on how to design component APIs with regards to raised/flat buttons and regular/multiline textfield, etc. That will dictate the naming.

@nathanmarks
Copy link
Member

In my own projects, I like keeping my code that isn't necessarily specific to React (ie, most of /utils) separate.

what about:

src/
  internal/ # shared/internal components
  utils/ # utils n shit
  Component.js
  ShitComponent.js
  index.js

(personally, I have /components and /utils at the same level -- but I understand that would overcomplicate the build step for root import paths here due to the relative imports in components)

@chrismcv
Copy link
Contributor

@nathanmarks fair enough... shall we open a separate issue for that, so as not to take this PR OT?

@nathanmarks
Copy link
Member

@chrismcv Yeah for sure, I was planning on opening it at some point tonight actually when I stop being scatterbrained.

@chrismcv chrismcv mentioned this pull request Mar 14, 2016
@alitaheri
Copy link
Member

@nathanmarks That can be achieved with static members:

class A extends Component {
  ...
}

A.B = class B extends Component {
  ...
}

@mbrookes
Copy link
Member Author

After much discussion with @nathanmarks, and subsequently with @alitaheri, here's what we've come up with:

  1. All public components in their own directory in src (as already planned, but not as part of this PR)
  2. All (shared) internal components in internal
  3. All public style / theme related files in styles
  4. Everything else in utils (including the non-public style utils, some of which are already there)

So:

src/
  Component1/
  Component2/
  Component3/
  [...]
  internal/
    InternalComponent1.jsx
    InternalComponent2.jsx
    [...]
  styles/
    getMuiTheme.js
    muiThemeable.jsx
    muiThemeProvider.jsx
    colors.js
    typography.js
  utils/
    util1.js
    util2.js

And in addition, that internal subcomponents that are collocated with their parent component go in an internal directory within that component directory, for example:

Tabs/
  index.js
  Tab.jsx
  Tabs.jsx
  internal/
    InkBar.jsx
    TabTemplate.jsx

This hopefully strikes the right balance between ease of import of public components, a meaningful structure and the least amount of visible change.

@nathanmarks
Copy link
Member

@mbrookes do you think we need the internal folders in components too? I'm not 100% on that -- might be noisy. Being tucked away in the subfolder already might be enough 👍

@alitaheri
Copy link
Member

@mbrookes @nathanmarks If we flatten the structure:

Tab/
  index.js
  Tab.jsx
  TabTemplate.jsx
Tabs/
  index.js
  Tabs.jsx
  InkBar.jsx

We might not need it. But what if we have other versions of each component?

Tab/
  index.js
  Tab.jsx
  closable.js
  stateful.js
  TabTemplate.jsx
Tabs/
  index.js
  Tabs.jsx
  scrollable.js
  stateful.js
  InkBar.jsx

Then it gets confusing! I like nested internal 😁

@mbrookes
Copy link
Member Author

I hadn't considered moving the public subcomponents out of their parent - I'll have to think about that one, but @nathanmarks the very reason for the inclusion of internal in any component directory that requires it was to solve the concern that having src/internal and not putting all the internal components in it was confusing.

Since we want to group non-shared internal components with their respective parents, putting them in an internal directory adds clarity, not noise.

@mbrookes
Copy link
Member Author

I think if we agree to it, I will tackle per-component internal separately anyway, so we don't have to decide that now, so long as everything else is acceptable.

@newoga
Copy link
Contributor

newoga commented Mar 16, 2016

One thing to consider in the internal folder vs no internal folder debate is where tests are going to reside.

I agree with @mbrookes though regarding not having to decide on the internal bit in our first round of migrations. Let's focus on fixing the public import paths first, and we can organize private stuff when we understand better how it's turning out.

@nathanmarks
Copy link
Member

@callemall/material-ui take a quick look at this

@mbrookes
Copy link
Member Author

@newoga - I was talking specifically about the per-component internal, which is not the main thrust of this PR.

Tidying up src will make sorting out the public exports more straightforward.

There's been lots talk, but no action. Lets get this done once alpha.2 is out the door.

@mbrookes
Copy link
Member Author

Closing in favour of #3749 which continues this.

@mbrookes mbrookes closed this Mar 21, 2016
@mbrookes mbrookes deleted the reorg-phase1 branch January 20, 2018 22:35
@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants