-
Notifications
You must be signed in to change notification settings - Fork 0
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
[107] Update button variants to match new variants in bitstyles update #113
[107] Update button variants to match new variants in bitstyles update #113
Conversation
…variants in bitstyles update #107
99df343
to
3eefeea
Compare
d386271
to
7a666d3
Compare
This adds a test to make sure the button component is backwards compatible. With bitstyles v5 we can no longer support the "reversed" attribute (at least not in the way it was done before).
61ec3aa
to
9d57c4c
Compare
@@ -64,7 +64,6 @@ defmodule BitstylesPhoenix.Live.Sidebar do | |||
JS.hide(to: "##{id}", transition: {"is-transitioning", "is-on-screen", "is-off-screen"}), | |||
"aria-controls": id | |||
) | |||
|> Keyword.put_new(:reversed, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how the sidebar looks without that button type though 🤔
While this is work in progress, it may already be helpful as a reminder.
...> """ | ||
""" | ||
<button type="button" class="a-button a-button--square a-button--round" title="Add"> | ||
<svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" class="a-icon a-icon--m" focusable="false" height="20" width="20"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the size 20x20 and the a-icon--m
really be a default, or is that just by coincidence the case on bistyles itself? cc @planktonic
I'd personally prefer to keep it empty. Users can still add the a-icon--m
class via the icon options if they wanted to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think it’s a good approach to not apply a size unless the user does that explicitly — the default icon size fits the default button size. (I’m working on icons over in bitstyles right now, so I’ll add “remove the size on buttons’ icons” to the list of tasks there 👍 )
Updating form 4 to 6 is quite something. Wouldn't it be better to go small steps? We want We spend quite some efforts to upkeep this and need to continue this for our running projects at least. We should talk about this again before continuing here... |
@andreasknoepfle yes, probably something we should discuss 👍 The main change is that bitstyles buttons now have two axis of variants — they can be different sizes, and different colors. One from each can be combined, but not multiple (small + primary or large + outline etc is good, but small + large or primary + outline makes no sense). Is the main change here that breaks backwards compatibility the switch to using two attributes? (one for color-variant, one for size-variant) We could keep using the single attribute to handle both, if that avoids the problem |
With jumping 2 major versions I can't say what are changes that break compatibility. But what I can say is that in this PR the tools that we have set up for maintaining compatibility have been ignored so far. Tbh I would probably start from scratch and just take this as an inspiration in order to migrate to 6.0 |
To be clear, these changes to buttons (using two combinable attributes, form and Color) were almost the only thing to change in v5 (from the PoV of the components. Many other technical changes happened on which future improvements can happen). So this PR could be set to v5 if that makes it easier somehow? I’ll take a look through and see if I can find out how we can use the backwards-compatibility functionality here, but any pointers you have would also be very welcome 🙏 |
I am not quite sure how the new buttons work (haven't checked yet), but what you want to do in general is map the classes of v5 to classnames of v4 in here: https://github.com/bitcrowd/bitstyles_phoenix/blob/main/lib/bitstyles_phoenix/bitstyles.ex and then use the v5 classes in the codebase on the various components. For now it was always possible to do this mapping like that, but if there is a more high level change needed in some component, you could access |
Thanks @andreasknoepfle, we’ll try the classnames first and see if it’s enough |
Superseded by #125 |
#107