New forms for tracker and graph creation

PropellantDyke commented 6 months ago

This PR continue #261

SamAmco commented 6 months ago

Sorry for being slow on this. I just want to re-assure you that I know this PR is here. I just need to find time to review it.

PropellantDyke commented 5 months ago

Hi! The big parts are done, but there is still a lot of small adjustment to do:

Form errors should be displayed only after pressing "Create" button
AddGraph form should get the same error system than in AddTracker form
Switches got too much of vertical padding
Switches should be colored according to the theme
Fix bug where numeric field are selected by default
Default value section should get a separator between the switch and the fields below
Fix vertical padding in "data conversion" inputs
Some inputs are still in the old way in AddGraph form (scale input, custom range and data filtering)
Add sections box in AddGraph to separate each parts

There is also some bigger changes to make, like breaking some forms into multiple smaller one, which is a classic approach to reduce the cognitive load when filling a form. But that's an another topic :)

Here is some screenshot of the actual result:

SamAmco commented 2 months ago

Hi @PropellantDyke

Today I found time to check out this branch and take a look at the new UI and the code. I apologise again for how long it's taken me, life has been pretty hectic lately.

First I'd just like to thank you again for your contribution. The code and the UI are looking brilliant in this branch, really well done! I think most people will be pleased to see the new cleaned up look when it's ready.

I hope you haven't given up working on it as I know you still had some improvements you wanted to make in your last post there. I would agree with most of that stuff and I do notice the odd component that looks a bit out of place or themed in a bit of an "old school" way compared to your new UI. There are also a few things I would add though:

Corner radius: You've introduced some new corner radius'. I think the trouble is when you have too many different corner radius' in an app it feels very visually inconsistent. You can go a long way to aligning the two pretty quickly by setting: <dimen name="card_corner_radius">4dp</dimen> to the same value as <dimen name="form_input_corner">12dp</dimen> but there are a fair few corners in the app that are not respecting this radius right now. It's not overly obvious when the radius is only 4dp but becomes a lot more obvious at 12dp. As a general point though try to not introduce new variables if you can re-use existing ones, it makes the app much more complex and hard to manage in the long run. I think it's best just to use (and potentially increase) card_corner_radius. If you want to rename it to something more sensible that's valid. We just want the minimum number of config variables.
Colours (design): I noticed in dark theme there's a new blue colour introduced that isn't seen anywhere else in the app (for example for the create/update button and the text field borders). I was a bit confused why you did this. In the existing app I think all buttons are orange regardless of theme (although it's a more muted orange in dark theme). If you wanted to introduce this blue I think it needs to be visually consistent across the app. However I think this new blue conflicts with the theme blue colours anyway as it's a different hue.
Colors (code): You have used a new pattern for defining colours that I think is out of alignment with the rest of the app. You created a new set of colors in values/colors.xml and defined a night colors.xml file. You will notice this didn't exist before yet the app supports a dark theme. That's because all of the dark theme colours are defined in the colors.xml file. In general when you want to use a colour you don't reference the colour directly like colorResource(id = R.color.form_primary_text), instead you use the theme e.g. MaterialTheme.tngColors.form_primary_text and then you add a colour to the TngColors data class and define it differently in LightColorPalette and DarkColorPalette. In fact in general the pattern should be that colour names are not associated at all with where they are used. So you use names like "gray", "white", "blue", for the colours and the theme attributes add the context of where it should be used. It's a little confusing in the code currently because most of the colours are defined in the xml but there are one or two defined in TnGComposeTheme.kt. That's because the app is in a midpoint of transitioning from xml to compose. Probably it makes more sense for now to put everything in xml and migrate it all at once at a later date. To use the xml colours in compose world via the theme you will need to change TnGComposeTheme. At a guess I think you will want to do something like this:
```
private fun lightColorPalette(
  material: Colors = lightColors(),
  selectorButtonColor: Color = Color.Magenta,
  myNewColor: Color = Color.Magenta
) = TngColors(
  material = lightColors(),
  selectorButtonColor = selectorButtonColor,
  myNewColor = myNewColor
)

private fun darkColorPalette(
  material: Colors = darkColors(),
  selectorButtonColor: Color = Color.Magenta,
  myNewColor: Color = Color.Magenta
) = TngColors(
  material = material,
  selectorButtonColor = selectorButtonColor,
  myNewColor = myNewColor
)

val TngColors.disabledAlpha get() = 0.4f

private val LocalColors = staticCompositionLocalOf { lightColorPalette() }

@Composable
private fun tngColors(
  darkTheme: Boolean,
  materialColors: Colors?
) = if (darkTheme) {
  darkColorPalette(
      material = materialColors ?: darkColors(),
      selectorButtonColor = colorResource(id = R.color.dark_gray),
      myNewColor = colorResource(id = R.color.dark_cyan)
  )
} else {
  lightColorPalette(
      material = materialColors ?: lightColors(),
      selectorButtonColor = colorResource(id = R.color.light_gray),
      myNewColor = colorResource(id = R.color.light_cyan)
  )
}
```
You will notice further down the theme provides the tngColors like so:
```
@Composable
fun TnGComposeTheme(
  darkTheme: Boolean = isSystemInDarkTheme(),
  block: @Composable () -> Unit
) {
  val (materialColors, typography, shapes) = createMdcTheme(
      context = LocalContext.current,
      layoutDirection = LocalLayoutDirection.current
  )

  val colors = tngColors(darkTheme, materialColors)

  CompositionLocalProvider(LocalColors provides colors) {
      MaterialTheme(
          colors = colors.material,
          typography = typography ?: MaterialTheme.typography,
          shapes = shapes ?: MaterialTheme.shapes,
          content = block
      )
  }
}
```
(some other changes will be required to get this working but i'm hoping you can infer those. This code is un-tested so I've guessed a bit what it will look like).

I'm actually hoping you don't need to define any new colours at all, just potentially theme attributes or hopefully just using existing theme attributes. For example form_primary_text is probably just MaterialTheme.colors.onPrimary.
Duration input: I would love it if the duration input style was unified across the app. You've introduced a new input style for durations which I like but can we use it (can we actually fit it as well?) in the data point input dialog?

If you get stuck with anything let me know and I might be able to just do it my self. Let me know as and when you want to merge . There are some things that would probably be good to address directly in this PR before merging, but there's also plenty of stuff I'm happy for you to put in a new PR. Stuff like aligning the dimens and colours files would be good to fix in this PR. Other than that I'm happy to merge.

Once again thanks for all your work and sorry for the slow response time. I hope you're still interested in rounding this off.

PropellantDyke commented 1 month ago

Thanks a lot for your feedback!!

My goal for this PR was to polish the UX of AddTracker and AddGraph form, while leaving intact the rest of the app. And then, in an another bunch of commits, make the whole app coherent. That's why I put effort to strictly separate new colors/widgets/radius from the the old ones, by using Form prefix. That way, I'm sure that I don't destroy something in an another page without knowing it. It also helps to ensure that I use a set a coherent functions and properties in the new code. But of course, this create code duplication and discrepancies in the app and this should be fixed before merging your 256_improved_trancker_creation_form to master. I hope it's okay for you, but let me know if you prefer a different approach.

Colors You are right about the blue that is out of nowhere. I just found it more appealing for dark mode, as tested with the mockup in my issue. And I admit that I've no clue about how was working the theme system, so thanks for the mini-tutorial! I've pushed a new commit where I've created a new FormTheme, derivated from TnGComposeTheme, and based a bit more on default colors.

Corners I've tried to follow what I see in most of the apps: buttons are 100% rounded, and fields got small rounded corners. In addition, when one or more fields are contained in a rounded box (like a section), I round the box a bit more so the circle center of the rounded corner of both parent and child (almost) match. Anyway, only 3 types of radius are needed for the new form, plus the older ones that are visible in the rest of the app until the new style is propagated everywhere. If old corner radius are still visible in the new forms, that's a bug or an unfinished work from me (like the "+" button to add a new graph line). If it's okay to keep separation between old and new code/theme for now, I don't see the need to modify the dimens.xml file.

Remember function I didn't know the existence of remember(), thanks for pointing that out :) I've fixed some of them. Some other are more complicated because apparently, remember() can't contain stringResource calls.

SamAmco commented 1 month ago

That all sounds very reasonable. Thanks for looking at those remember calls as well. I'm happy to merge this in then if that's how you'd like to proceed? I have also been doing a little work on master so i can merge that into the branch as well soon and then you can work off the latest if you branch back off of 256.

PropellantDyke commented 1 month ago

Perfect, you can merge it!

I will try to separate things in different PR now that the core of the new form is available in the branch.

SamAmco commented 1 month ago

Merged! .. I will merge master into this branch soon and let you know here when that's ready here.

SamAmco commented 1 month ago

Ok feature/256.. should now have the latest from rc-v4.0.0 so feel free to keep working off that.

SamAmco / track-and-graph

New forms for tracker and graph creation #262