aepp-components icon indicating copy to clipboard operation
aepp-components copied to clipboard

button type plain missing

Open dadaphl opened this issue 6 years ago • 8 comments

All button types were refactored to constant TYPE_PROPERTY_VALUES [1]. Seems like the button type pain got lost. As the the plain type is still in the design [1], I wonder if it was on purpose. Please have a chat with @raymundjacobs if it is still needed.

[1] https://github.com/aeternity/aepp-components/blob/c1ddb16e83fb2b7754bf139fe05159beaa913494/src/constants/index.js#L1-L6 [2] https://app.zeplin.io/project/59e4cf99fc0bb2f99a89551b/screen/5a1d6df0d860f2097e40e23a

dadaphl avatar Apr 03 '18 10:04 dadaphl

@dadaphl , @davidyuk, do you remember why in https://github.com/aeternity/aepp-components/pull/28 plain was removed as a possible value of type and added as separate property?

acdbaykal avatar Apr 03 '18 12:04 acdbaykal

I remember talking about this a while ago with @davidyuk . So that we can turn make every button type into a plain one. So: ‘Normal Plain’ will have a transparent background + black text. ‘Exciting Plain’ transparent bg + exciting font colour. ‘Dramatic Plain’ transparent bg, with a dramatic font colour. This may help?

raymundjacobs avatar Apr 03 '18 12:04 raymundjacobs

Hi @raymundjacobs,

thanks for the explanation. Do you still have use for this feature? And is this not very close to invert?

acdbaykal avatar Apr 03 '18 12:04 acdbaykal

@acdbaykal yes, at several places where we need a "link-like" button. For instance in the sign transaction and login screen. 01 10-base-login

raymundjacobs avatar Apr 03 '18 15:04 raymundjacobs

@dadaphl , I would prefer to move all the plain state of aeButton into a new component like aeLabelButton. I already have lost track of all the possible prop combinations and resulting appearances aeButton can have. Other than that I don't think there is anything to do for this issue. Just maybe adding more examples with the plain property, to make it's functionality clearer.

acdbaykal avatar Apr 05 '18 21:04 acdbaykal

@acdbaykal If I understand correctly, plain just means the background is transparent. The type color will be used as font color instead of background color. If thats is true, i don't see why we should replicate aeButton as it will be the same, just with a different visual appearance. Do you have a suggestion how to avoid duplicate code between aeButton and aeLabelButton if we would go with the aeLabelButton?

dadaphl avatar Apr 05 '18 21:04 dadaphl

I have provided necessary examples with plain property here: https://github.com/aeternity/aepp-components/commit/931d0f92a6f545698cb1bcef462a191f2679a9b6#diff-3e31aad88f87ab1b8c37cfd6afb16e5e. But for some reason, these examples right now mixed with examples of inactive property: https://components.aepps.com/#ae-button. I think examples will be more clear if we won't mix various properties in examples for one of them. For example, https://github.com/aeternity/aepp-components/commit/8dbe35d8008e53f8ea243c0d251fe5bc56ac2bbe#diff-3e31aad88f87ab1b8c37cfd6afb16e5eR22 is better than https://github.com/aeternity/aepp-components/commit/8dbe35d8008e53f8ea243c0d251fe5bc56ac2bbe#diff-3e31aad88f87ab1b8c37cfd6afb16e5eR92. AeButton and possible AeLabelButton have a lot of common functionality and right now plain property implemented in enough simple way, so I think what we shouldn't extract AeLabelButton component. If you want to make AeButton simpler I suggest to get rid of block and invert properties.

davidyuk avatar Apr 06 '18 03:04 davidyuk

don't get rid of invert and property, its there for a reason.

block property also has a purpose, although one could argue that its the parent components job to override that.

dadaphl avatar Apr 06 '18 14:04 dadaphl