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

Supporting Shadows and Rounded Corners on Images and Buttons #221

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

hoffmabc
Copy link
Contributor

This is a proposed idea based on a need we have on our project.

I've first pushed code to demonstrate for right-menu buttons, but this could apply to any image and/or layer and left menu as well.

It supports borders and drop shadows.

For your component you must specify certain attributes in the style section.

These are the ones supported by my code submission. There are defaults for all except the shape, which of course by default would just be the normal square shape.

"style": {
          "shape": "circle",
          "border_color": "#ffffff",
          "border_width": "2",
          "border_opacity": "1",
          "shadow_color": "#000000",
          "shadow_width": "2",
          "shadow_opacity": "0.5"
      }

An example of this in action is:
image

@hoffmabc
Copy link
Contributor Author

CC'ing @drwasho as well as he is working on this with me.

@gtramontina
Copy link

I like the idea. How about using properties that resemble CSS, such as border-radius (I think this is already being used with other elements) and box-shadow

@gliechtenstein
Copy link
Contributor

Like @gtramontina mentioned, I think instead of using shape, it's better to use corner_radius (not border-radius though), since it's already used everywhere else to describe rounded corners. Also it's also more flexible because you can also describe slightly rounded corners as well as a full circle.

That said, when it comes to shadow-related attributes, I actually like @hoffmabc 's approach.

The reason I didn't follow a lot of CSS attribute naming conventions (although resemblant) was because a lot of them are needlessly complicated and confusing, which is understandable because the people who worked on the standard basically made them up as they went, which is natural for a new piece of technology.

box-shadow is actually a great example of this, because there's no way to specify the color of a box-shadow separately. You must declare them in one line, like box-shadow: 10px 10px 5px #888888;

I always hated how I have to always look up the order of these things to get them right, every single time (I still have to, and I'm sure most of you guys do too). With Jasonette we have a great notation which frees us from having to remember the exact order (because the order of the keys doesn't matter in JSON), and I think we should take full advantage of it. So the bottom line we should make sure we have a style attribute for every single styling property.

For this reason I don't think we need to always follow the css naming convention.

To summarize:

  1. I think we should use corner_radius for the rounded corner
  2. I'm fine with everything else

p.s.

One thing I'm concerned about though: I'm not sure if i will have time to work on the Android version immediately...

@gliechtenstein
Copy link
Contributor

One more thing I just came up while thinking about box-shadow, if we're going to implement shadows, maybe we should cover all of what box-shadow covers (or at least design it and name the attributes with the assumption that we will expand to support all of the box-shadow attribute equivalents eventually).

For example the current proposal doesn't have a blur or directional attributes, but if we do end up implementing shadows, wouldn't people expect those naturally?

@hoffmabc hoffmabc changed the title Rounded right-menu buttons w/shadow+border Supporting Shadows and Rounded Corners on Images and Buttons Jun 23, 2017
@hoffmabc
Copy link
Contributor Author

Ok I broke out the shadows and rounded button to helper methods so they can be reused. I cleaned it up and followed the conventions you set above, adding the blur and offset stuff for shadows.

I also added the left menu. I'm going to go forward and add in support for other places as well.

@gliechtenstein
Copy link
Contributor

Sounds great! Btw thanks for resurfacing the left menu, I originally had implemented the left menu but decided to bury it because I couldn't figure out some edge cases.

I think one was that the left menu automatically turns into an X button when a view is opened as a modal, so there's a potential collision there if the modal view already has a left menu button. One use case I'm excited about is that using Jasonette you can create an app that lets users customize their own page, so I thought this may become problematic.

I was thinking about this and couldn't make up my mind so decided to just launch without it, and renamed the right menu as menu.

  1. Anyway, I think the concern above might have been a bit of an extreme overthinking, and now thinking instead of being afraid of these cases we could just leave these up to the developer and "best practices". Let me know if anyone has thoughts or ideas on this.
  2. Also, these were previously named left and right respectively, and you could access them using $jason.body.header.left and $jason.body.header.right, consistent with how $jason.body.footer.input works. If we were to incorporate this into core, I guess we could deprecate the menu keyword from the documentation (but still support them indefinitely) and use right to refer to the right menu.

Would love to hear thoughts, or if I'm not making sense, feel free to ask me to clarify. Thanks!

@gliechtenstein
Copy link
Contributor

@hoffmabc I am trying to merge this in. But as always, I want to make sure what we have here on the iOS side is in line with the Android counterpart. Which means we'll need to implement some of the features not yet implemented on the Android side. I do plan on going through the code but when you have time could you give a quick summary of the current status of this PR, in terms of the features? That would be super helpful. Thank you!

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.

3 participants