NYPL / staff-picks

1 stars 3 forks source link

Kp/www 268 put tags in ul #213

Closed ktp242 closed 6 years ago

ktp242 commented 6 years ago

This PR wraps the tags inside a <ul> and updates each of them into an <li>. Because of the change, there are some DOM and styles need to be updated too. Of course with related tests.

However, as we add a <ul> here, the wrapper of <ul> can't be a <p> anymore. And it affects the way the line of tags breaks when there's not enough width of the column.

With enough width screen shot 2018-05-17 at 3 28 36 pm

Without enough width screen shot 2018-05-17 at 3 30 52 pm

courtneymcgee commented 6 years ago

@ktp242 - Can we add &nbsp; between tags? Does that help? Apologies for the naïve question!

ktp242 commented 6 years ago

@courtneymcgee what kind of visual do you want to achieve? A bigger white space between Drama, and On the dark side, and etc? If so, we can totally do it either by adding more white spaces or just increasing margin to style rules.

EdwinGuzman commented 6 years ago

I'd say the updated margin which is there is more maintainable than the &nsbp;.

ricardoom commented 6 years ago

definitely better to add margin than a &nbsp;

ricardoom commented 6 years ago

oops

courtneymcgee commented 6 years ago

I'm fine with a margin update. I'm just looking to have the tags listed right after "Tags:" and not start on a separate line. If there are lots of tags, it's ok if they continue on to a separate line.

ktp242 commented 6 years ago

Follow on what @courtneymcgee said, should we just have the tags on a separate line then? @ricardoom

ktp242 commented 6 years ago

Updated based on the comments! Please see the screenshot for the new updates!

screen shot 2018-05-18 at 12 53 38 pm