search

GrupaMedialnaSalwator / kompas.sds

App for families to visit salvatorian places
MIT License
3 stars 0 forks source link

Info screen refurb #62

Closed knutm92 closed 2 years ago

knutm92 commented 2 years ago

fixes #41

knutm92 commented 2 years ago

Thank you for the review, I will implement the suggestions in the coming days.

knutm92 commented 2 years ago

@darowsluk I refactored the code a bit, mainly added wrappers for Text. I'm not entirely convinced by the way I created them so further suggestions are welcome. p.s. it seems that the group photo needs some adjustments so I will add it via another PR once ready.

darowsluk commented 2 years ago

@knutm92 Here are my suggestions to the text widget wrappers:

  1. File text_styles.dart was generated by Supernova from Figma file, so I think it is best not to change its contents manually. I suggest creating a seperate file with just the KompasText wrapper class.
  2. I recommend making just one KompasText() class with a parameter called 'style', which will take an enum to select a style from AppTextStyles, namely: headerH1-6, paragraphText, etc.
  3. With this mechanism in place we can create a separate issue to refactor all Text() calls into KompasText(). This will enforce consistent text styles throughout the entire app.
knutm92 commented 2 years ago

@darowsluk Good ideas, I have pushed the changes and created a refactoring issue, please have a look.

darowsluk commented 2 years ago

@knutm92 The refactoring of KompasText() looks great. One problem that I had was with the name of two imports: import 'package:kompas/statics/kompasTextWrapper.dart'; After I changed it to "kompasText.dart" everything builds fine.

knutm92 commented 2 years ago

Repaired :)

darowsluk commented 2 years ago

Now everything is OK. :) Let's merge.

knutm92 commented 2 years ago

Merging is blocked, could you send a review with an approval?