search

Semantic-Org / Semantic-UI-React

The official Semantic-UI-React integration
https://react.semantic-ui.com
MIT License
13.22k stars 4.05k forks source link

Migrating to Semantic-UI-React #243

Closed levithomason closed 8 years ago

levithomason commented 8 years ago

Semantic-Org maintains a collection of official integrations with various libraries. There is a missing spot for React. Per this thread, Stardust will fill this hole and become the official Semantic UI React integration.

This issue will serve as the place to hash out all the moving parts of making the migration. As concerns and solutions are raised, I'll keep an updated list of TODOs for migration here. Once complete, we'll point everything to the new location.

There is great mutual benefit to both TechnologyAdvice and Semantic-Org in this merge. Big thanks to @jlukic for the idea.

TODOs

This is an organic list, based on the comments in this issue:

TechnologyAdvice has put a large amount of time and money into Stardust. We'd like to retain attribution through the merge. We're open to ideas on how this is accomplished. Our initial thoughts were along the lines of an unobtrusive link in the footer of the README and/or doc site.

Permissions

TA must retain admin permissions to the repo. We use Stardust in production and need full rights to the source at all times. This also applies to repo settings. We have bot users and other integrations that we'd need to maintain.

Project Direction

Though the project will continue to be developed as a community effort, a @TechnologyAdvice representative will continue to lead the project. Currently, that is @levithomason.

First Steps!

Since @jlukic has been through this a few times, love to hear your thoughts on the first steps to moving things over to Semantic-Org.

cc @athurman @davezuko @Fluidbyte @kyleturco @stephen-ta @TomFrost @thomasruns

Please let me know if I've overlooked anything.

dvdzkwsk commented 8 years ago

Would be great to also solidify the roadmap and start adding real milestones as part of this. I know you have really solid ideas for a stable release, and that there is a doc, but I think part of this migration could involve really clarifying the direction with concrete details. Would also help us identify super easy "first contribution"-type issues that could draw activity and contributors.

levithomason commented 8 years ago

Couldn't agree more. The existing ComponentGuidelines.md are very outdated. The ROADMAP.md needs updated as well. Adding todos.

nicbarker commented 8 years ago

We're using semantic and react quite heavily in our new product over at ManageFlitter - I'd love to contribute to the development of this project once there are some defined guidelines for community contributions.

levithomason commented 8 years ago

@nicbarker awesome, we'll be moving this way very quickly. In the meantime, you could take a look at the source for the <Label /> (simple component) and the <Dropdown /> (complicated component).

These two components were developed in line with the component API pattern chosen here, see API #3. All component APIs will be moved to this pattern. Lastly, you can also try out the components and view their source on the doc site.

We'll get all this info condensed into an easy to follow guide soon, thanks for reaching out.

nicbarker commented 8 years ago

Great, I'll start having a read.

jlukic commented 8 years ago

@levithomason I've given you admin rights to Semantic-Org/Semantic-UI-React, feel free to port over the existing project.

When you think things are at a stable state where I can evaluate some components let me know. If everything checks out, I can look into providing a sub domain on our DNS linking react.semantic-ui.com to host any documentation.

levithomason commented 8 years ago

Sounds great. We'll get to this through next week I'm sure.

jlukic commented 8 years ago

I also want to mention previous react integrations and those who have worked on them. Perhaps some of the contributors to those projects might want to lend some feedback.

If any of you guys see this, please feel free to contribute your opinions about the direction of Semantic-UI-React and the components created here.

yordis commented 8 years ago

@jlukic Like I told you, there is some changes from SUI itself or we will fighting the CSS framework too much or we will create ugly implementations. Like the accordion component, SUI is very opinionated in how you create your HTML markup, in this case thing like have .header + .content + .header + .content is something that React don't like it, in this case every Item should leave inside some wrapper.

This is not something that HAVE to be like that but is what React likes to do, also features like closedNested is something that Flux architecture will fix for you if you chose to do something very flexible in the structure of the accordion.

levithomason commented 8 years ago

@yordis I'd love for you to link to (or post) any of your findings and issues with the Accordion on our Accordion Issue. I've looked at the markup there and it seems doable. Though, devil is in the details and I haven't implemented this one yet.

I did however accomplish similar functionality with the <Item /> view (not the List "item"). This component, as you likely know, has very conditional markup depending on how it is used. The pattern we are using is to accept the "parts" of a component as props, then we can build any markup necessary depending on whether or not the user is using that "part" of the component. Parts here meaning, header, content, item, etc.

We're also not attempting to reuse micro components like <Content /> as react-semantic did. SUI does not deal with these in a consistent enough fashion to do so. We found that explicitly constructing each component's markup, based on prop "parts", allows us to generate any markup.

All said, I would still really love to have your issues with Accordion detailed/linked on our Accordion issue.

levithomason commented 8 years ago

I started work on the Accordion just to see the issues, if any. I don't think it will be a problem. Ideally, the title + content would be wrapped in another element, however, we can use an abstract AccordionPanel component. This was done with the abstract <TableColumn /> as well.

levithomason commented 8 years ago

In light of https://github.com/TechnologyAdvice/stardust/issues/243#issuecomment-220519397 I'd also like to ping @asvetliakov and @hallister from semantic-react. We've had some good conversation about SUI + React and considered merging work at one point. As with the others, any input or contributions welcome.

asvetliakov commented 8 years ago

Semantic react is based on three ideas:

  1. Conditionally rendering based on contextTypes (for example menu sets context type and if you're rendering header as child of menu it will have proper markup for it). We're believing this better and cleaner approach than that you've used in stardust.
  2. Component augmentation. <Dimmable component={Form} blurring {...formProps}/> for example. Semantic-react also is supporting structured component approach in few complex elements: <DropdownMenu menuComponent={(props) => <Menu {...props} />}
  3. Explicit prop types. I don't remember which way you went, but in my opinion this better:
<Header emphasis="primary" stacked/>

than this:

<Header classes="primary stacked"/>

First variant clearly defines API and could give autocomplete/documentation hints in editors/ide

yordis commented 8 years ago

@levithomason In SUI there is too much something > something CSS selectors which force you to use the exactly the same HTML markup. We should wrapper .title + .content in some element from a proper React component but (at least in that time) I couldn't do such of thing because SUI do constrains in the HTML markup

yordis commented 8 years ago

Also, if you take a look to the Nested Accordion, SUI remote the class ui from the nested one, why? That shouldn't happen, just add complexity and is not component friendly IMO. Now you will have to do special cases to the component.

yordis commented 8 years ago

Dropdown component is another one that doesn't wrap the trigger elements and take a look to the CSS selector for the stylings, you have to put it right inside the dropdown component, you can't wrap it, which again for React components just add complexity to the implementations, in this case, is easier to attach anything to the trigger area/element without do it in the dropdown itself and then filter the target or actually in the render function filter the children and have special cases.

levithomason commented 8 years ago

@yordis Thanks for the examples. I agree entirely about these pain points and difficulties.

While there are CSS inconveniences as you've well noted, they are not blockers. SUI core and SUI-React can both iterate to improve these things over time. The alternative is to not have a SUI-React library. The endgame for me is that we get a SUI-React library and there isn't one.

levithomason commented 8 years ago

@asvetliakov We're on the same page with points 2 and 3 and SUI-React will have these qualities. See #208 for API discussions.

As for 1, using context as a primary design decision. Could you elaborate on what exactly is better about this approach? I don't see that it adds any developer features, user features, or performance improvements. It does however introduce instability, complexity, and a loss of features:

  1. context is documented as highly experimental with lots of warnings against it's use. Using it so heavily goes against the bold red warning in the docs, "If you have to use context, use it sparingly.". If this API changes, or worse goes away, then semantic-react will no longer work.
  2. There are several types of Headers in SUI, each with varying capabilities (size, icons, etc). By putting them all in one class you are polluting props and validation. Though you can use context to conditionally choose a render method, you cannot do so with propTypes.
  3. It introduces a lot of unnecessary complexity and computation. Example, with separate *Header components you explicitly include or don't include a ui class in the render method. By combining all the headers in to a single class, you need shouldHaveUiClass which has a cyclomatic complexity of 10 and must be called every render.
  4. You cannot create slim custom builds. Logic and markup for all components is spread out all over the codebase. With dedicated components, you can truly isolate component logic and markup.
  5. Context is a type of global, which inherently means the render method is no longer a pure function of props. Reasoning about and testing the code becomes much harder. When a single component can be many different things in many different contexts, it is hard to track what is going on. Whereas, a dedicated component does the same thing, every time.
  6. I think you're also bound to run into multiple parent conflicts. There is no reasonable way to predict what hierarchy an app will end up with. A context dependent child component may have several contextual parents that could incorrectly calculate the render result.

While I really like the idea of conditionally rendering child components based on the parent, I can't justify it. Aside from having fewer child components, what benefits are there that outweigh the issues outlined above?

yordis commented 8 years ago

@levithomason also we should't replicate every feature that comes form a CSS framework into some Javascript behavior, what I mean with this is actually draw a line between styling and behavior; for example: stuff like inverted all over the place in SUI shouldn't be a props at all at anytime. That's just for styling propose and that's why CSS exists, if not what's the idea, going back in the future where you create your stylings throw properties? We killed that long time ago.

So from this we should actually think deeply about the SUI components.

yordis commented 8 years ago

@levithomason About improving the core of SUI I spoke to @jlukic about some of those ideas, specially in the dropdown component, where you even detach the menu section from the dropdown itself which makes easier for SUI itself to separate the concerns about that component because for me a Dropdown is just Menu + Trigger components where the Menu should display in relative position to the Trigger ... And some other ideas, but this is something that even SUI team should be open to change IMO, I don't want to be fighting with SUI or ending on creating my own SUI

yordis commented 8 years ago

Consolidated from SUI things like ui loading form which basically creates a special case for the loader inside the form, I prefer to think that I just need a regular loader instead of actually delegate that spec to the form, I think is more component friendly and we use composition in a better way in this case.

hallister commented 8 years ago
  1. context has been documented as highly experimental for years. If the API goes away, react-router won't work either. The API has been stable since, what, 0.13? We're on stable semantic-versioning with React now. That means API changes require a major release. It's an odd argument, because API changes happen in every piece of software and they always break things that depend on that API. What happens if SUI changes a class name? With your approach I know have to change it every where in my app vs a single place.
  2. No, there's one type of header in SUI. It has the class header. All other forms of the header are classes in addition to the header. Varying capabilities is effectively synonymous with props. That's why they exist. You are effectively making 8 components because they have different props. Your API surface area right now is almost the size of ours, having implemented about half what semantic-react is.
  3. Cyclomatic complexity... It loops over the top level children of a Header component. Right now it loops twice (although that would be easy to fix to once) over an array of objects with a maximum of ~8 children. Why are we talking about a maintainability index for a function that's incredibly easy to follow?
  4. Who cares? It's a component library that's entirely dependent on a styles library that's very tightly coupled. Your "slim" build still has to inherent that tight coupling from SUI, and complex interfaces will still require that you use multiple components. So you are pretending to be loosely coupled while actually being tightly coupled. When was the last time you needed only the <Header> component from SUI? And really, how slim can your builds be with jquery and the entire lodash library as dependencies?
  5. Maybe. But I have multiple running instances of semantic-react currently in production. These aren't complex interactions and there are still a (very small) finite number of outcomes since (bottom line) we are talking about a small variation in class names.
  6. Haven't yet, nor have there been any bug reports for that particular issue. Again, I'm using this in production where as stardust is effectively a collection of elements. That makes it difficult to compare because we are comparing an actual useable product to an extremely small, extremely limited set of React components that basically map 1:1 with actual HTML.

At the end of the day, stardust is a whole lot of theory with little actual work. Seems odd to be so critical of a project that you approached to potentially merge with. Particularly when you've accomplished so little since then.

yordis commented 8 years ago

Seems odd to be so critical of a project that you approached to potentially merge with. Particularly when you've accomplished so little since then.

@hallister who are you referring to?

levithomason commented 8 years ago

@yordis regarding props for CSS and behavior. This sounds solid, I'll think about this some more and get some thoughts back to you. Off the cuff, I really like the idea of not having unnecessary logic and tests for converting style props to classes when they can just be className to start with. It could introduce confusion about when to use props vs className in some cases. Also, declarative props are ideal to work with for component logic. Still, I think this is a great point and I want to mull it over and look at some specific components with this in mind.

Regarding core SUI improvements, also agreed. I do think we could make these changes as we go though. I think if we wait for the core updates, it could be a long time before we are able to move forward.

I'm not sure I follow your comment about the form. Feel free to elaborate a bit if you'd like to discuss it further.

EDIT

@hallister is referring to me about previous discussions of merging. See here https://github.com/hallister/semantic-react/issues/62.

yordis commented 8 years ago

@levithomason about the form, if you take a look to the loader in the form is created using actually a class instead of actually add inside the form a loader component, more component base, the form doesn't know about a loader component that can't be draw nicely inside.

asvetliakov commented 8 years ago

@levithomason Are you trying to me point me that this:

<Header content={<contentstuff/>} />
<Header image={<imagestuff/>}/>

Along with similar stuff for modals

<Modal header={<headerstuff/>}/>
<Modal imageHeader={<headerStuff/>}/> // need ui class in header for header in modal with image content, so say hola! to another prop in your way
<Modal iconHeader={<headerstuff/>}/> // same as above

Better than this?:

<Modal>
   <Header/> // without ui class 
</Modal>
<Modal>
  <Header icon="test"/> // ui icon header
</Modal>
<Modal>
  <Header> // header with image content need ui class
     <Image src="test"/>
  </Header>
</Modal>
<Modal>
  <Header> // same as above
     <Icon name="test"/>
  </Header>
</Modal>

Me rio. I'm not trying to be aggressive but in this case you need either to add much noise to props for complex components (menus, accordion, dropdown, modals etc...) or work with semantic folks to fix ui class in child markup.

levithomason commented 8 years ago

@hallister It is tempting to correct what's been said about Stardust, but I don't think that is going to help anything but my own ego. You and the semantic-react team are invited here because I appreciate your code chops and collaboration. I'm going to return to discussing the design decisions of the official SUI-React library. I'd love for you to join me in doing so.

With that, let me reset the focus. In this thread we're comparing ideas. The best of which will be implemented into the official library. Right now, we're talking about why context is (or is not) superior to explicit child components. The result of this conversion will sway the direction of the official library heavily. Continuing:

  1. It's true the trajectory of the context API seems like it has a positive future. In order to consider the other points, let's assume this is a non issue.

  2. The issue here is how to separate props and prop validation for the different use cases. We don't want to loose validation. I also don't think a component should render improper usage of it's API.

    Your comment did spark an idea. The advantage of using context is having a single reusable component. The advantages of having multiple components are global free and separation of concerns (including prop validation). It's likely possible to create a composable Header that could accomplish all of these things. Context and branch logic would not be required but there would still be a single Header component. Maybe I'll toy with this a bit. Would like to know any trials you may have already investigated in this area.

  3. The point here is that with context additional logic and computation is required to render. If we go with separate child components that have dedicated roles, this extra logic computation is not required. There is a win here for not using context. I don't see any downsides in this area to using separate components.
  4. The goal here is about limiting awareness and separating component features as much as possible. I think it would be ideal to allow custom builds of the JS. This is very doable if there are not interdependencies.
  5. (and 6) We all agree components that are pure functions of props are preferred. I assume we also agree that globals are bad, generally. We'd rather have pure components that are global free. The intent here is to point out we can do this if we don't use context. I don't believe this can be achieved with context.

So far it seems the main advantage of using context is that child components can be defined once and reused. This is preferred. I'm wondering if it should just be accomplished using composition rather than context. This may be a way of gaining all the benefits of both approaches while also removing the downsides.

Per the checklist in this issue, I'll get more docs regarding the direction of the official library. For now, here are some quick clarifications:

levithomason commented 8 years ago

@asvetliakov I'm not sure where those code sinppets are from or what you're laughing about ("me rio" => "I laugh"). But, here's a the correct comparison between the current approaches:

semantic-react

<Modal>
   <Header/>
</Modal>

SUI-React

<Modal>
  <Modal.Header />
<Modal />

@asvetliakov @hallister You were invited here out of my respect for our previous conversations and collaboration. Not sure what happened. Currently, you're both jumping to conclusions and approaching this with hostility. It doesn't look good and is also not welcome. You're both still invited, and I'd love to have you contribute to the ideas and direction of the official SUI-React lib. If you'd like to decline that invitation, please do so explicitly and let's move forward. If you'd like to accept that invitation, please say so and let's get back to the shaping the library.

asvetliakov commented 8 years ago

@levithomason Aplogize me for my hostility

levithomason commented 8 years ago

No grudges 🙉 . You have some good ideas, i want them all in one lib :)

asvetliakov commented 8 years ago

I don't think that your approach is good (i.e. implementing <Modal.Header> - is it HOC component or another header from scratch? ). In my opinion this is leading to duplicating components and increasing complextivity (i don't think that maintaining 3-4 headers is good stuff). Your main advantage with separating components and including only few of them is not a good trade off (in semantic-react you can include only few components too, but it's not a big deal in my opinion if Modal will come with few more component definitions). At least we all developers and we want a good, intuitive and maintable API.

Looping over childs in render component is not a big deal too. JS is pretty fast and with modern hardware i'd prefer do not care. You're trying to do optimizations before completing main logic in my opinion (premature optimization is the root of all evil - Knuth). Saying more, there is a react reconcilation system and if we'll use shallow compare or functional components, most components will just skip future renders.

At final note, i'm really glad that semantic-ui will have official library. But i can't agree with your design solutions.

levithomason commented 8 years ago

...<Modal.Header> - is it HOC component or another header from scratch? In my opinion this is leading to duplicating components...

Nothing is in stone yet, we'll soon decide. We've explored 2 approaches so far and I'd like to try a 3rd as (mentioned above, similar to a HOC) before deciding. I want to see if it is possible to achieve all 3 of these things 1) reuse 2) accurate prop validation 3) components that only generate supported SUI component markup.

Consider the menu header, it does not support the majority of features of a regular header. Specifically a header inside of a Menu cannot have:

These are not officially supported but will kinda work if you add the ui class and wrap content:

This means a generic Header will generate unsupported SUI Menu header markup. Even if more branch logic is added to the className buildup to prevent invalid classNames based on Menu child context, the Header will still be unable to provide prop validation. There is no way to change prop validation based on context. In either case, this is complexity.

A Header and Menu header have almost nothing in common beyond the header class.

This is true for other headers. Card headers do not support most Header features but you can have text alignment (maybe more). List headers only support a div with the header class. It is hard to imagine why a ListHeader shouldn't just be a div with a header class.

A dedicated <Header /> is capable of blindly and safely implementing all SUI Header features, providing correct prop validation, and producing only valid SUI Header markup (good things!). The same is true for a dedicated <Menu.Header />, etc. This also removes all branch logic requirements (complexity).

I don't think it is good to continue adding more branch logic to handle Card child, List child, etc. Prop validation is lost, component capabilities are leaked, and it is considerably more complex. The gain is you can use a single "Header" instead of "Card.Header" in your render function. This seems like an illusion of reuse as it reuses all capabilities while excluding the majority of them with branch logic in the reused cases.

This detailed debate has been great. It has raised a lot more issues, ideas, and considerations. The intent behind using context and branch logic is good, reuse. I still think the novelty of a single Header is very appealing, even if it doesn't add up on all counts. I'm gonna look closer at reuse through composition while not sacrificing prop validation, encapsulation, and simplicity. I'll report back here with my findings.

At final note, i'm really glad that semantic-ui will have official library. But i can't agree with your design solutions.

This kinda saddens me, I don't even know what the design solutions are yet ¯\_(ツ)_/¯

levithomason commented 8 years ago

@yordis thanks for the form clarification given here https://github.com/TechnologyAdvice/stardust/issues/243#issuecomment-222012090. This also makes a lot of sense to me. Will let this cook as I think all this over.

levithomason commented 8 years ago

We're using semantic and react quite heavily in our new product over at ManageFlitter - I'd love to contribute to the development of this project once there are some defined guidelines for community contributions.

@nicbarker we've now got solid contributing guidelines and a nice road map. Have a read through the new README, especially the "How can I help?" section. It links out to our CONTRIBUTING guide, task lists, "help wanted" issues, and example contributions.

We have several people contributing now, would love to hear you join us.

Cheers!

layershifter commented 8 years ago

@levithomason seems, there is nothing that stops us from migration 👍

levithomason commented 8 years ago

Indeed, I'll start this next week (tomorrow). There are some things not on the list, like deleting Semantic-Org/Semantic-UI-React and transferring this repo. Then, we'll need to scrub the code base and remove all "Stardust" references. Lastly, I'll need to get with Jack to figure out how the docs will be hosted at react.semantic-ui.com. Next week will be a good week for this project :D

levithomason commented 8 years ago 
in stardust/ on master 
› npm deprecate stardust "Install semantic-ui-react instead."

Woop!

levithomason commented 8 years ago

Repo transfer process started https://github.com/Semantic-Org/Semantic-UI-React/issues/3

levithomason commented 8 years ago

All set, there is some wiring left to do with some of our services. Will handle those as separate issues.