SurLaTable / slt-ui

:books: An end to end solution for bringing React components to your legacy system.
https://surlatable.github.io/slt-ui/
GNU General Public License v2.0
9 stars 1 forks source link

Feature/store-locator #37

Closed LeonardMeagher2 closed 6 years ago

LeonardMeagher2 commented 6 years ago
LeonardMeagher2 commented 6 years ago

@ryanpcmcquen Fixed the typo, but you're welcome to change the react styleguidist info to your personal preference in another PR. Also CodeSandbox doesn't work as far as I know, so it doesn't make much sense to have in the readme.

ryanpcmcquen commented 6 years ago

@LeonardMeagher2, this doesn't work for you? https://codesandbox.io/s/github/SurLaTable/slt-ui/tree/0b63e06d14b42cc5f91739dff90a09ed747b1e93

LeonardMeagher2 commented 6 years ago

@ryanpcmcquen That's a specific commit and doesn't work if you change it to develop, and the statement Conversely, you can also hack on components using CodeSandbox implies you get to work on update to date components which isn't true.

ryanpcmcquen commented 6 years ago

We should reword that verbiage, but the value of having an instantly clickable view into the ecosystem is a great way to show people what we are doing, and it will be updated soon enough.

LeonardMeagher2 commented 6 years ago

@ryanpcmcquen I don't think as a developer (who is also our user) an outdated version of a library is very useful, the link is commented out not deleted, so if support for codesandbox gets better it's easy to uncomment. At the moment the link leads to not current code so what's the point of actually using it.

ryanpcmcquen commented 6 years ago

I guess I see it as destructive, since we are removing something that appears for the end user. It's not ideal, but it also isn't a complete misrepresentation of our library.

ryanpcmcquen commented 6 years ago

I think the key thing to consider here, is whether or not the decision to remove this link belongs in a branch for the Store Locator component.

ryanpcmcquen commented 6 years ago

@terryfritschSLT, can we get your eyes on this PR? I've fixed a few of the docs issues and I think it is ready to merge, although it does not represent a complete Store Locator component, it does include the changes mentioned in @LeonardMeagher2's original comment.