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

fix: button icon variant #128

Merged
merged 5 commits into from
Feb 24, 2020
Merged

fix: button icon variant #128

merged 5 commits into from
Feb 24, 2020

Conversation

shibulijack-fd
Copy link
Contributor

@shibulijack-fd shibulijack-fd commented Feb 21, 2020

  • Fixed buttons with icons.
  • Fixed toast message button icon issue.

More info:
Button variant is not directly linked to icon variant. For instance, button of variant danger must display icon with secondary variant. Hence it requires a mapping.

Also, the toast-message component uses ember-cli-flash under the hood, which returns specific types such as "Info", "Warning" etc.. While we've followed similar naming conventions in our component, there exists a small problem of capitalisation. To fix this, there are 2 possible solutions:

Solution#1: Helper
Create a helper which capitalises the first letter.
Time complexity: O(n)
Space complexity: O(1)

Solution#2: JS Map
Create a simple mapper like:

const VARIANT_MAP = {
  'Info': 'info',
  'Success': 'success',
  'Warning': 'warning',
  'Danger': 'danger'
};

Time complexity: O(1)
Space complexity: O(n)

Although the helper seemed like a straight-forward solution at first, creating a modifier function is an overkill for a small component which does not even reuse this functionality. Hence, I'm going with the faster solution.

@shibulijack-fd shibulijack-fd merged commit 50b9644 into master Feb 24, 2020
@shibulijack-fd shibulijack-fd deleted the nucleus-v1-fixes branch February 24, 2020 12:10
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.

2 participants