Closed Wmaileq closed 3 years ago
The math in the key insights cards feels off. The left-most one says a 31% drop on a large number of people but then says it's a reduction of only 10. The second card also seems off (if it's really a drop of 84 people then it should be more around ~40%). Are all of those numbers coming directly from the data file (the big current number, the change number, and the percentage change) or are any of those being calculated in the app? If they're being calculated here there may be a bug:
This data comes directly from sources and is not calculated in the app. I've looked through the data files and noticed that current version of data file consists of real and fake values. So I can assume that fake values were used here.
Thanks for letting me know, @Wmaileq! Sounds good on that point. However, you marked all of my comments from my last review as resolved even though they haven't been and my last review was more recent than your last commit. Did you perhaps make changes but forget to push the commit up to the repo? I just did another local test to be sure and the issues I called out in my most recent review are still apparent: my inline comments from the review, the mobile overflow issue, and the changes in the requirements on the issue that need to be taken into account.
@jessex I am working on a dynamic font size change in dependence on window width. I am trying to do a dynamic font size component to prevent text overflow. Is it OK if boxes, where text overflows, will have a lesser font size than the others? Doesn't it feel off if font size will differ from card to card depending on the content?
Is it OK if boxes, where text overflows, will have a lesser font size than the others? Doesn't it feel off if font size will differ from card to card depending on the content?
That's a good point! If we reduced the font size for all of the text in the metric blocks, that might be a better solution (rather than trying to change it only if it overflows).
@Wmaileq Feel free to reduce text size across the board based on @hobuobi's comment above, but obviously it's going to be impossible to get a font size that works for all content and all screen resolutions, so let's just try to make a best effort that ensures that longer text (e.g. larger numbers or "Not available") don't overflow on mobile layouts.
Made card font size dynamic on mobile devices (320-439px)
Description of the change
Type of change
Related issues
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: