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

feat: extend functionality of the tabs component #99

Closed
wants to merge 2 commits into from
Closed

feat: extend functionality of the tabs component #99

wants to merge 2 commits into from

Conversation

maylukas
Copy link

@maylukas maylukas commented Dec 12, 2018

I tried to use the Tabs component in a small demo / test app and found some aspects that might need changing:

  • The tab sizes with the content which is due to rendering the content within the header item.
    image
  • The state of the tabs component can't be controlled externally and also there is no callback to observe any changes
  • The component will always rerender since the tabs are passed as a list (the reference changes every time). If the tabs would be passed as children, the change detection of react would work
  • Rendering non-text content fails
  • The interface feels a bit non-react-idiomatic :) Passing the tabs as children with their respective content seems to fit better (maybe just personal taste).

I implemented another tabs component as a suggestion and would be happy if you could give feedback on this.

There are also some things I probably didn't get right:

  • I probably didn't follow your style guide because I didn't find a reference to it :) If you let me know, I will fix my issues (maybe provide an editorconfig?).
  • I used typescript since I saw the issue regarding a possible change of the implementation language to typescript Typescript Support  #32 so I figured I could provide an example. If you want me to use javascript I can change that (switching to typescript / providing type definitions would be great though :) )
  • The playground doesn't work / needs to be implemented as I didn't quite figure out what was required to control the props of children.
  • The tabs component does not support overflow content as I wasn't sure how this should be done according to the Fiori 3 guidelines (is there an officially available reference?). It would be possible to overflow the tabs if the number of tabs exceeds a certain amount (e.g. passed as a prop).
  • I didn't include the react-router integration as I am not sure if this is the right place for it. The choice of the routing framework (or a lack thereof) should be up to the downstream application. Let me know what you think about this.

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2018

CLA assistant check
All committers have signed the CLA.

@CodesOfRa
Copy link
Contributor

Also check @chrismanciero 's branch for typescript https://github.com/SAP/fundamental-react/tree/typescript-conversion

@maylukas
Copy link
Author

maylukas commented Dec 13, 2018

Ah cool stuff! I wasn't aware that this is already in progress. I am going to rebase onto the typescript-conversion branch.

@maylukas
Copy link
Author

Should I create the pull request against the typescript-conversion branch? Or is it planned to merge the typescript branch into master any time soon?

@CodesOfRa
Copy link
Contributor

I think it's best to create a PR against typescript-conversion branch, there is still a debate regarding TypeScript vs Flow .

@maylukas maylukas changed the base branch from develop to typescript-conversion December 17, 2018 08:18
@maylukas
Copy link
Author

Okay. I changed the base.

@maylukas maylukas closed this Dec 19, 2018
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