stevesims / govuk-frederic

https://stevesims.github.io/govuk-frederic/
MIT License
3 stars 1 forks source link

CounterBar needs a refactor #14

Closed stevesims closed 6 years ago

stevesims commented 6 years ago

Our current CounterBar component has a few problems that need to be addressed. We now understand better how this component will be used, and thus better get how it should be put together.

Right now we have several requirements that will necessitate changes to the CounterBar component, and possibly other companion components.

stevesims commented 6 years ago

Coloring score boxes will require allowing component that provides those boxes to allow such configuration and that value to be passed through

stevesims commented 6 years ago

We could potentially completely rewrite the component so that it works differently - maybe as a container with sub-components that the using component can use as a construction kit, rather than explicitly providing a list of counters - this may be a better option....

stevesims commented 6 years ago

Ideally we don't want to have react-router as a dependency, so we should pass in the link component as a prop, probably providing a default implementation that uses a plain a tag - although that may be unnecessary if we're doing a complete rewrite as a different component architecture could render this void

gavinorland commented 6 years ago

Hi Steve, Your first two comments here, I think I could begin work on introducing this capability to the current component (I have created a branch and could go ahead if you like and then show you). The second two comments, I wonder if you could elaborate a bit on what you mean...

gavinorland commented 6 years ago

I've begun by looking at the elements and wrapping. I have converted the CountWrapper and Counters to be anchors, and been able to remove duplicate style rules by making the latter use the former.

Thinking about the wrapping, it seems to me we can:

  1. Set a percentage width for these elements, and use MQ breakpoints to change the number of columns. Possibly long labels might wrap within each column.
  2. Set flex-wrap to wrap elements to next line if not enough room. Trouble is, if flex is set to 1 to make the items also expand to fill horizontal space, then ones dropping to a lower row can have very wide widths
  3. Simply use inline-block. This way the items always collapse to their content size and wrap automatically, but they don't expend to fill screen width. This also stops titles wrapping under the numbers. If it's not essential for these items to expand/wrap tightly to either end, I think this is the simplest solution.
stevesims commented 6 years ago

My thoughts on width is that we shouldn't use percentage. Managing that via MQ breakpoints feels problematic, and whilst with our current use case we know how many elements we're likely to have that number is not guaranteed.

We can however use min and max width values. That would allow us to still have the items flex to grow, and also for items that wrap onto a next line not getting too wide.

The design we have is essentially full-width. The individual elements (score + title) need to not wrap, grow to accommodate their contents if necessary, and expand (within reason) so that the width of the screen gets filled.

We have no guidance about wrapping. I would suggest however that in an ideal world, the individual score elements should wrap within their own container, with the total sitting to the left of that. i.e. something like:

8 Total   1 Title  2 Title  3 Title
          4 Title
gavinorland commented 6 years ago

Right - if we can use min and max width values and have some idea of what they could reasonably be, that'll no doubt make things a lot easier. Will tinker with this and what might work - including the total being aligned to the left. As soon as we have a good solution for this I will also think about the restructure you mentioned on Slack. Did you mention something about the optional colours on Appear.in? (Audio is never great on there!)

gavinorland commented 6 years ago

If we're allowing these to expand via flex to fill available width on row 1, I think it's going to be very hard to get lower elements to match these widths without using columns/breakpoints. They'll always be a bit different. We can cap max-width (here 200px), but that stops row 1 elements from being able to wrap tightly to edges:

screen shot 2018-07-10 at 11 19 24

When there are many, we can see the lower ones not necessarily matching width of higher ones as they hit their max-width:

screen shot 2018-07-10 at 11 19 43

So this is quite tricky. But you can see I have the counter text wrapping down beside the counter number, which I think will be right.

I'm thinking it might be impossible to get lower elements to match the arbitrary width of higher ones without using JS or columns/breakpoints. Here's an option using float: left and a fixed width for comparison:

screen shot 2018-07-10 at 11 53 37

I think this may be the way we have to go if we want uniform widths. Still thinking about it - will see about getting the total on the left next anyway!

gavinorland commented 6 years ago

Here's another example using Flexbox and limiting elements to a max-width of 300px.

screen shot 2018-07-10 at 13 17 29

Thing is we kind of want the one on the bottom left to be limited (and lower than this), but not the one on the right! Anyway, will concentrate on getting total to be separate now...

gavinorland commented 6 years ago

Counter now aligned left. This is with flex option as above.

screen shot 2018-07-10 at 13 57 39
gavinorland commented 6 years ago

This is the same sort of thing but with inline-block and a fixed width. I'm thinking it might be the most workable solution, but it doesn't go full width.

screen shot 2018-07-10 at 14 07 10
stevesims commented 6 years ago

Looking good.

I think the version with flex (but not inline-block) looks best - having the content stretch out to go full width does IMHO fit our original design a bit better.

gavinorland commented 6 years ago

Morning! I've refactored this hopefully along the lines you expected and pushed up two commits to the branch. Will mention in the scrum, but maybe you can take a look. The thing that is not yet implemented is the click handling. We might need stateful components for this - or the Context API! I wanted to get your thoughts before proceeding. TTYL

gavinorland commented 6 years ago

If it's decided this needs any further work in future - re. layout behaviour for example - am happy to revisit.