Conversation
Signed-off-by: Vihan Bhargava <> Signed-off-by: Vihan <vihan+github@vihan.org>
saagarjha
left a comment
saagarjha
left a comment
There was a problem hiding this comment.
Hi Vihan, I'm glad to see that you're using break and are interested in contributing to it! Unfortunately, I have largely been managing break in a way that's not particularly conducive to outside contributors, so you're going to have bear with me as I clean it up and get it ready to receive changes (yes, I had some commits that I just force pushed. You should be able to cherry-pick your commit cleanly onto the new master; let me know if you need help doing this). From an administrative point of view, what this will probably look like is that once your commit is clean and ready to land, I will add in boilerplate and manually merge it in. I'm kinda picky about certain things, sadly:
- I know you need to change the bundle identifier and development team in project.pbxproj and Info.plist locally to make this compile, but you don't need to include these changes in the pull request.
- break currently uses tabs for indentation, and it would be nice if you could as well for consistency.
With regards to the UI changes, I'm enthusiastic about the direction to change the default CSS, which I have come to realize is kind of bland. A couple of points I wanted to discuss:
- I think we can actually expand the scope of this to all the webviews in the app. Are you interested in styling assignments and news as well, at least to keep them consistent with the LoopMail message view?
- I think we discuss the CSS a bit, and how we plan to apply it. In particular, we have access to the standard system font classes and colors and might want to look into seeing if any fit. This might require folding the CSS into our list of UI constants. I do like the look overall though.
- I'm not sold on propagating the changes from the message view to the list of LoopMail as well. My take was that the list of LoopMail is purposely kept plain because it doesn't actually contain much content, to mirror what many iOS system apps do. Plus using non-standard UI makes dealing with AutoLayout/Dynamic Type/OS-specific tweaks just a bit harder.
Anyways, I'd love to hear what you to think and see what you have to contribute to break!
| tableView.deselectRow(at: indexPath, animated: true) | ||
| } | ||
|
|
||
| override func tableView(_ tableView: UITableView, heightForRowAt indexPath: IndexPath) -> CGFloat { |
There was a problem hiding this comment.
This should already be set in the storyboard, AFAIK, since the table cells already rely on AutoLayout. Did something not work for you?
There was a problem hiding this comment.
From my testing it looks like both the Table and the Table Cells need auto-layout/auto-sizing, this seemed to be enabled for the table but the table cells seemed to use a fixed custom height in the storyboard
There was a problem hiding this comment.
I'm not sure I understand you here. The layout of the table itself should be managed by UITableViewController; I'm not doing anything special here and it should expand to fill the screen. I have marked the table view with an automatic row height in the storyboard file:
This works for me. Is this not what you are seeing?
|
|
||
| static let dateFormatter: DateFormatter = { | ||
| let dateFormatter = DateFormatter() | ||
| dateFormatter.doesRelativeDateFormatting = true |
There was a problem hiding this comment.
I'd like to hold off on this change for now. It's not that I wouldn't like this being added, but I want to check where else I can add this and have it be a separate commit.
|
Regarding the CSS/System styles I think we could probably make some system which allows us to bridge system UIFont to CSS styles... I'll take a look at this... |
|
Also depending on how much HTML is used something like Stencil could be used to move beyond inline HTML strings and repetition of HTML but that might add a non-trivial amount to bundle size |
|
Yeah, I don't think we need something as complicated as Stencil for a simple use-case like this; string interpolation should be good enough to handle this. With regards to bridging |
2 participants
I'm using break and it's great as it has a lot of features but there a few things I thought could be improved so I thought I'd make a few PRs.
This PR modifies some UI aspects for the loopmail VCs. Screenshot:
Anyways, I hope to contribute more over the following weeks and kudos to the well doc'd code :)
Signed-off-by: Vihan vihan+github@vihan.org