chore: add docs with size #17279
chore: add docs with size #17279
Conversation
J-Son89
requested review from
ilmotta,
alwx,
cammellos,
FFFra,
flexsurfer,
tumanov-alex,
briansztamfater,
smohamedjavid,
clauxx,
OmarBasem,
ibrkhalil,
codemaster115,
BalogunofAfrica,
mohsen-ghafouri and
ulisesmac
Jenkins BuildsClick to see older builds (20)
|
|
|
||
| ```clojure | ||
| ;; good | ||
| (defn button |
There was a problem hiding this comment.
Fixed indentation:
(defn button
[{:keys [size]}]
[rn/view
{:style {:height (case size
20 20
20 40
0)}}]
...)
(defn button
[{:keys [size]}]
[rn/view
{:style {:height (case size
:size/s-20 20
:size/s-40 40
0)}}]
...)
src/quo2/README.md
Outdated
| To avoid having the codebase littered with magic numbers we instead have a keyword convention | ||
| to use in components to map these keywords with their sizes. | ||
|
|
||
| The convention is `:sizes/s-<number>`, e.g size `20` is `:size/s-20` |
There was a problem hiding this comment.
Probably a typo, :sizes/s-<number> => :size/s-<number>
There was a problem hiding this comment.
Yep a typo, thought i fixed before. Will adjust!
There was a problem hiding this comment.
just wondering why s- is needed?
There was a problem hiding this comment.
it's a little bit confusing, like sizes s,m,l,..etc
There was a problem hiding this comment.
Yes the exploding keywords is the purpose of /s-. If it's okay I will adjust to @flex-surfers & @ulisesmac suggestion. While the keywords are slightly magic it is a better alternative than what we had and really the best approach is for semantic names but these need to be in the design file so we're slightly too far gone from that for now.
We can of course talk to designers for this going forward.
I can make the required convention changes in the codebase in this pr
There was a problem hiding this comment.
If it's okay I will adjust to @flex-surfers & @ulisesmac suggestion.
Sure, that and because :size/20 is considered invalid by the Clojure reader, so :size-20 is better.
There was a problem hiding this comment.
I also dislike :size/20 or :size-20 because it's more like a glorified magic value
Yes, I think at the end it's the same as 20, I see almost the same in:
{:foo "bar"
:size 20}than
{:foo "bar"
:size :size-20}But it's ok, I prefer :size-20 over :size/s-20 👍
Thank you so much @J-Son89 for documenting this! :) 👍 💯
There was a problem hiding this comment.
Done, and in this pr I also changed the uses in the codebase. I think a search and replace should be safe here 👍
src/quo2/README.md
Outdated
| @@ -69,6 +69,38 @@ In the image above we can see the properties are `Type`, `State`, `Size`, | |||
| ...) | |||
| ``` | |||
|
|
|||
| ### Handling Sizes | |||
| For sizes there is a slightly special case. In the designs, sizes are referred to as integers. | |||
There was a problem hiding this comment.
When reading the guideline in isolation, the first sentence seems unnecessary. We can start the paragraph with In the designs, sizes ....
Suggestion, with line breaks at 80:
In the designs, sizes are referred to as integers. To avoid having the codebase
littered with magic numbers we instead have a keyword convention to use in
components to map these keywords with their sizes.
src/quo2/README.md
Outdated
| [rn/view {:style { | ||
| :height (case size | ||
| 20 20 | ||
| 20 40 |
There was a problem hiding this comment.
40 40 probably? i know its just an example but anyway
There was a problem hiding this comment.
Actually that's a typo 😅 thanks!
src/quo2/README.md
Outdated
| To avoid having the codebase littered with magic numbers we instead have a keyword convention | ||
| to use in components to map these keywords with their sizes. | ||
|
|
||
| The convention is `:sizes/s-<number>`, e.g size `20` is `:size/s-20` |
There was a problem hiding this comment.
just wondering why s- is needed?
src/quo2/README.md
Outdated
| To avoid having the codebase littered with magic numbers we instead have a keyword convention | ||
| to use in components to map these keywords with their sizes. | ||
|
|
||
| The convention is `:sizes/s-<number>`, e.g size `20` is `:size/s-20` |
There was a problem hiding this comment.
it's a little bit confusing, like sizes s,m,l,..etc
src/quo2/README.md
Outdated
| To avoid having the codebase littered with magic numbers we instead have a keyword convention | ||
| to use in components to map these keywords with their sizes. | ||
|
|
||
| The convention is `:sizes/s-<number>`, e.g size `20` is `:size/s-20` |
J-Son89
force-pushed
the
jc/sizes-doc
branch
from
8f8160c
to
667a37e
Compare
i don't think it's worth it to give so much attention and time to this, i would leave it as is with just integers, but if we change i still prefer "global" :size-24, because it will be confusing which one you have to use with component , so as a developer i preffer the simplest one when i use component, lets say now its simple, [button {:size 24}] i don't need to think much, i know any component has size prop with int value |
|
I agree with the simplicity mindset brought by @flexsurfer. At the same time, my heart goes to a codebase where pretty much all arbitrary design constants (sizes, colors, spacing, dimensions, etc) are both:
I worked in a codebase with designers and devs respecting this approach and there's no way I prefer magic design constants everywhere like what we do in BUT, going back to what @flexsurfer said, I think we shouldn't overstress these ideas, because in the end, it's the design team that would need to more or less radically change how they prefer to handle these values in the design system, and I don't think such changes are possible at this stage of the game. |
|
Sure, simple sounds good. 👍 |
J-Son89
force-pushed
the
jc/sizes-doc
branch
from
667a37e
to
171b62c
Compare
@J-Son89, I approved the PR with the intention of solving the following problems:
Therefore, I think a global keyword like It's a marginal improvement, but it's good nonetheless. About contextualizing keywords, it would be good to have a separate discussion because this PR had a different scope. What problems are we trying to solve by contextualizing size keywords? We should be careful how we do this, because where before we had a single size |
86% of end-end tests have passedFailed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
yes, I agree this goes beyond the initial purpose of the pr. I just didn't want to cement another decision without raising my last concern. I agree that this is not the optimal solution but a reasonable tradeoff for our current setup. |
|
Hi @J-Son89 thank you for PR. No issues from my side. Ready to be merged |
@VolodLytvynenko I'm taking care of this PR since Jamie took some days off. I will merge it then 👍 |
ulisesmac
force-pushed
the
jc/sizes-doc
branch
from
171b62c
to
df2f522
Compare
* chore: update guidelines for sizes * chore: update to size uses in codebase to follow new convention --------- Co-authored-by: Ulises M <ulises.ssb@hotmail.com>
Added docs for recent decision made by theme on how to handle sizes in the code base
To test:
This pr updates the use of this new api for a set of components. Nothing should change.
these components include:
Group avatar
Icon avatar
Network dropdown
Preview list
System message
Settings/ Data item
Context tag
Network tags
Number tags
Wallet keypair