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

Tabs Component initial #85

Merged
merged 3 commits into from
Apr 10, 2019
Merged

Tabs Component initial #85

merged 3 commits into from
Apr 10, 2019

Conversation

daigof
Copy link
Contributor

@daigof daigof commented Apr 9, 2019

  • Similar to Form component this is the initial version compatible with PocketDerm. However:
  • I dont like how it handle the Tabs as object {id,title}, worse it couples the initialTab to Id=1 forcing us to provide at least one Tab object with Id=1
  • I propose drop the tab objects and just work with arrays of string titles and the active Id would be the index now. As usual this would require more refactor in PocketDerm as well.

@snags88 snags88 temporarily deployed to curology-radiance-pr-85 April 9, 2019 16:13 Inactive
@benkolde
Copy link
Contributor

benkolde commented Apr 9, 2019

@daigof

You can totally refuse this but on mobile, tabs become left aligned with the first string having a margin of 24px from the left side of the window. It would be nice to bake this into the component if possible.

@snags88 snags88 temporarily deployed to curology-radiance-pr-85 April 9, 2019 18:27 Inactive
@daigof
Copy link
Contributor Author

daigof commented Apr 9, 2019

You can totally refuse this but on mobile, tabs become left aligned with the first string having a margin of 24px from the left side of the window. It would be nice to bake this into the component if possible.

@benkolde Yeah absolutely, I refactor all the styles a little bit. Note that the Tab Item already had a padding left/right: 24px so I didnt add the extra margin-left, means that the Tab first letter will be exactly 24px to the left of the Tabs component container

docs/tabs.md Outdated Show resolved Hide resolved
docs/tabs.md Outdated Show resolved Hide resolved
src/shared-components/tabs/test.js Outdated Show resolved Hide resolved
@daigof
Copy link
Contributor Author

daigof commented Apr 9, 2019

@snags88 what do you think regarding flattening the tabItems structure to be just an array of strings (titles) and handle tabId as array index. Having refactored the Tabs Example I realized the tabItems object structure may be due that the objects contains part of the content displayed in page but I still think this should be decoupled.

@snags88
Copy link
Contributor

snags88 commented Apr 9, 2019

@daigof i think we can keep as objects for now. I briefly looked at how we use it for pocketderm and it seems like we use the object notation to help with setting the query string.

@daigof daigof merged commit 6d923c4 into master Apr 10, 2019
@daigof daigof deleted the new-tabs-component branch April 10, 2019 13:54
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.

3 participants