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

Add Spacers #66

Closed
wants to merge 4 commits into from
Closed

Add Spacers #66

wants to merge 4 commits into from

Conversation

hrules6872
Copy link

πŸ“‘ What does this PR do?

Add Spacers to start creating our own design language

βœ… Checklist

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

πŸ§ͺ How can this PR been tested?

🧾 Tasks Remaining: (List of tasks remaining to be implemented)

πŸ–ΌοΈ Screenshots (if applicable):

@exallium
Copy link
Collaborator

This took me more than a while to reconcile with myself on, because when I see a bunch of top-level functions like this the first thing I think is that we're going to end up polluting our class-space very quickly.

Are there opinions on utilizing some kind of namespacing here?

object Spacers {
  @Composable
  fun XLarge(modifier: Modifier = Modifier) {} // ...
}

// Usage
Spacers.XLarge()

This has the upside of making it easier to find things. If I want a specific spacer I can do Spacers. and get a nice list.

I can be convinced either way, just a random musing.

import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentHeight
import androidx.compose.foundation.layout.wrapContentSize
import androidx.compose.foundation.layout.*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@hrules6872
Copy link
Author

@exallium although I see your point, I can't decide for myself at the current stage of the project. Let's see what the community says πŸ˜€


@Composable
fun RowScope.WeightSpacer(modifier: Modifier = Modifier) {
Spacer(modifier = modifier.weight(1f))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general - I think it's best to not couple components with scopes, unless there's a lot of useful reusable logic for that commonly used scope. Both weight spacers here are simple enough that IMO they can just be inlined. If you wanted a weight beyond fill (1f) then it's the same amount of code to call this component then to just use the Spacer composable directly. Also - not something I care about or feel strongly, just thinking out loud here in case this was overlooked

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use weight() outside of those scopes.
For me it has is more readable and cleaner this:

Row() {
  Text()
  WeightSpacer()
  Text()
}

than that:

Row() {
  Text()
  Spacer(modifier = modifier.weight(1f))
  Text()
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agreed, but that's why I mentioned anything besides default weight will make it moot.

WeightSpacer(modifier = Modifier.weight(2f))

Spacer(modifier = Modifier.weight(2f))

}

@Composable
fun XSmallSpacer(modifier: Modifier = Modifier) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't see anything wrong with using hardcoded DP values in compose for spacing instead of wrapping them in another composable, as long as the values conform to the 4 dp material grid sizing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that this is a cross-platform project :)
Hard-coded dimensions are not an option, even for a simple Android app, if you want it to be compatible with all kinds of devices.

}

@Composable
fun XSmallSpacer(modifier: Modifier = Modifier) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend adding a doc comment with the actual spacing size in it to all of these.

/**
 * Size: `2.dp`
 */
@Composable
fun XSmallSpacer(...) {
  // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that'd come useful πŸ‘

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#66 (comment)

I'm not sure what you're trying to say with this link? Using pixel dimensions is not good of course, but DP are totally fine (which is what you have here anyway). Admittedly, I haven't worked very much with Compose Desktop, but I have used dp dimension values when I did.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a first approximation so I hardcoded all the dimensions.

Dimensions (like paddings) must be defined in the Theme in order to support all kind of devices/platforms.
So, we shouldn't put this kind of comments because they can be a lie :)

Copy link
Member

@crocsandcoffee crocsandcoffee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not entirely clear to me what these custom Spacers are really solving, I think it might make more sense to have these implemented as modifiers instead.

However, I don't feel super strongly and I don't see any harm in getting them merged in for now. With that being said - I think adding some paddings to the theme and having these Spacers (or Modifiers if you choose to go that route) update to pull the padding from the theme instead of hardcoded dp would be a good POC to hopefully showcase the benefit a bit more.

Approving anyway, as it can be done in a follow-up PR.

Thanks and nice work πŸš€

@hrules6872
Copy link
Author

@crocsandcoffee I started to use that kind of custom Spacers in one of my side projects, so I thought it was a good and easy PR to start contributing to this project. Then it has become not as good and easy as I thought at the beginning πŸ˜…

The good thing is that I have started using namespaces for this kind of functions (as you have pointed out to me in the comments). I am very grateful to learn from you guys 🫢

That said, I have no strong feelings about this PR and I guess it's best to close it and forget it.

@hrules6872 hrules6872 closed this Dec 6, 2022
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.

None yet

7 participants