Skip to content

Wrap iconAfter slot in same span.icon-container on icon slot#1219

Open
nucleogenesis wants to merge 1 commit intolearningequality:developfrom
nucleogenesis:kbutton-iconAfter-container
Open

Wrap iconAfter slot in same span.icon-container on icon slot#1219
nucleogenesis wants to merge 1 commit intolearningequality:developfrom
nucleogenesis:kbutton-iconAfter-container

Conversation

@nucleogenesis
Copy link
Member

Description

I found while trying to setup some [<- Prev] [Next ->] type buttons that only the icon slot had a wrapping span w/ a class that sets a top value and relative position but the iconAfter does not have it. This meant that I'd need to have two separate classes with two separate values to align the icons but fixing this makes that easier and makes KButton more consistent.

(Possibly) Breaking changes in Kolibri

Here is everywhere I found that uses the #iconAfter prop - some definitely have applied styles to address the problem that this PR fixes and would likely be able to be simplified quite a bit.

Changelog

  • Description: Wraps KButton iconAfter slot in same span.icon-container that icon slot is already wrapped in.
  • Products impact: bugfix
  • Addresses: -
  • Components: KButton
  • Breaking: yes - styles may be broken in some cases
  • Impacts a11y: No
  • Guidance: Identify places where the #iconAfter slot is used in Kolibri and update styles as needed

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

vertical spacing being inconsistent demands separate styles when
tweaking the icon's position by having to address the `iconAfter`
differently than the `icon`
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.

1 participant