Closed iainkirkpatrick closed 4 years ago
A few things that I am noticing
... continuing
[x] Search field without border or background area - could be intentional but a bit funny as inconsistent with other search fields
[x] Download Data button not aligned with other menu items
Addressing your points @tmfrnz:
Intro images are stretched horizontally on wide screens - there must be a limit to how far these images can stretch. I had discussed this with @adi-cam that this limit could be at 1440px
I'm reverting this - I can see what was done was mistaken in reading Adina's notes, but I'm not confident of a working solution in the short-term
Download Data button not aligned with other menu items
This looks to me to be the same on the current live site - I'm wavering between seeing it not aligned and then not sure if it's just my eyes playing tricks on me. From a look in the inspector there doesn't appear to be any reason they aren't aligned... something IMO to look at later.
slider cards very high (minor)
I believe this was the solution presented for the text causing different card heights (esp with different languages) - perhaps could use some tweaking later, but for now seems fine?
x-axis chart label too wide for mobile (single metric - works on single country)
I haven't seen any mobile designs for this... IMO there's a few issues with general legibility + clarity of the data on mobile, especially narrow mobile screens. We could remove the label on mobile, but that's probably not really what we want?
All the other points that I haven't quoted I've also now pushed fixes for - should be a single commit for each of them (in order). So for me, the main one is the last quoted point above re x-axis chart labels on mobile - is this a dealbreaker to getting the changes in? Happy to discuss / work on this tonight with you @tmfrnz 🙂
Also just found that the message "FAQs" now appearing in the aside has no translations associated with it - I've removed these for now so it defaults to "FAQs" across all locales.
Thanks very much @iainkirkpatrick and @tmfrnz for identifying and ironing out these issues.
Just a couple of things I noticed:
for this
x-axis chart label too wide for mobile (single metric - works on single country)
I would suggest to simply remove the worse/better annotation that is overlapping
Addressing your points @tmfrnz:
Intro images are stretched horizontally on wide screens - there must be a limit to how far these images can stretch. I had discussed this with @adi-cam that this limit could be at 1440px
I'm reverting this - I can see what was done was mistaken in reading Adina's notes, but I'm not confident of a working solution in the short-term
Yes this has always been a challenging layout proposition so will have to introduce a limit somewhere where images stop aligning with the screen edge
Download Data button not aligned with other menu items
This looks to me to be the same on the current live site - I'm wavering between seeing it not aligned and then not sure if it's just my eyes playing tricks on me. From a look in the inspector there doesn't appear to be any reason they aren't aligned... something IMO to look at later.
fixed
slider cards very high (minor)
I believe this was the solution presented for the text causing different card heights (esp with different languages) - perhaps could use some tweaking later, but for now seems fine?
Sure
x-axis chart label too wide for mobile (single metric - works on single country)
I haven't seen any mobile designs for this... IMO there's a few issues with general legibility + clarity of the data on mobile, especially narrow mobile screens. We could remove the label on mobile, but that's probably not really what we want?
So actually the issue is the overlap with underlying better/worse labels, so have removed them for mobile. However one could consider removing them altogether as the new axis label kind of conveys the same message
All the other points that I haven't quoted I've also now pushed fixes for - should be a single commit for each of them (in order). So for me, the main one is the last quoted point above re x-axis chart labels on mobile - is this a dealbreaker to getting the changes in? Happy to discuss / work on this tonight with you @tmfrnz 🙂
I think this is good to go now - have a few other little things to address today so can address any (small) potential issues that have not yet been identified
Just a couple of things I noticed:
Download Data button not aligned with other menu items
This looks non-aligned to me. But agree that it's not a major and can be addressed later.
Fixed =)
- On the Safety and Empowerment tabs on the country pages, the "Source" note is very close to the bottom of the charts. If the gap could be increased slightly (to be more like that for Quality of Life), that would be great. But not a major.
Might be able to have a look later...
@iainkirkpatrick
Haven't noticed this before
Quite ugly I think, so for now I am changing this to
@iainkirkpatrick, some more issues coming up so I will keep documenting here
react_devtools_backend.js:6 Warning: React does not recognize the
borderSize
prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasebordersize
instead. If you accidentally passed it from a parent component, remove it from the DOM element.react_devtools_backend.js:6 Warning: React does not recognize the
borderColor
prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasebordercolor
instead. If you accidentally passed it from a parent component, remove it from the DOM element.
@iainkirkpatrick, some more issues coming up so I will keep documenting here
- [x] do not use borderSize, borderColor as attributes (instead use styled-components or style attribute)
react_devtools_backend.js:6 Warning: React does not recognize the
borderSize
prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasebordersize
instead. If you accidentally passed it from a parent component, remove it from the DOM element.react_devtools_backend.js:6 Warning: React does not recognize the
borderColor
prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasebordercolor
instead. If you accidentally passed it from a parent component, remove it from the DOM element.
Adding some IE 11 issues here (that I will not be able to address today unfortunately)
also see https://github.com/HumanRightsMeasurementInitiative/hrmi-dataportal/issues/146
Quite ugly I think, so for now I am changing this to
- align key right
- show 'symbol' on right also (after all this is where it is shown also
- do not wrap source
- use common label (replacing hard-coded text labels
Thanks @tmfrnz. Looks good.
Latest design tweaks from @iainkirkpatrick and @bsides44
Let me know if there's anything unclear / I can do and I'll fix on Wednesday NZT 😄
Deployed at https://hrmi-design-tweaks-3-1.firebaseapp.com