Skip to content
This repository was archived by the owner on Mar 17, 2023. It is now read-only.

Icon only add button #85

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Icon only add button #85

merged 4 commits into from
Apr 5, 2022

Conversation

rolandpeelen
Copy link
Contributor

@rolandpeelen rolandpeelen commented Sep 9, 2021 •
edited
Loading

📔 Description

📝 Checklist

  • All user-facing changes have changelog entries.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Screen Shot 2021-09-09 at 4 18 21 PM

@rolandpeelen rolandpeelen marked this pull request as ready for review September 9, 2021 13:48
@rolandpeelen rolandpeelen requested a review from mavam September 9, 2021 14:18
mavam
mavam approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left a suggestion to use a more powerful type instead of a flag, but with my limited ReasonML foo, consider it optional. (pun intended)

src/Tabs.re Outdated
~depth: int,
~standalone,
~theme,
~iconOnly=false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would model this as something along the lines of Option<addText> rather than adding a boolean flag. I'm not sure if this is possible, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - that makes sense :)

@rolandpeelen
Copy link
Contributor Author

@mavam - I think this one can be merged without any issues :)

@mavam mavam merged commit 1d69776 into master Apr 5, 2022
@mavam mavam deleted the topic/icon-only-add-button branch April 5, 2022 11:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mavam mavam mavam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rolandpeelen @mavam