updates to investment status component #424

feedmypixel · 2017-08-07T16:59:24Z

Updates to the markup and styles in the investment status component

Multipart PR, part of base branchfeature/investment-project-stages and PR #395

tyom · 2017-08-08T11:25:41Z

I wonder if we can use meta-item for this. We use it in investment project list items.

teneightfive · 2017-08-08T08:48:49Z

src/templates/_components/investment-status.njk

@@ -14,28 +13,18 @@
 #}

 {% if projectCode %}
-  <div class="status-bar grid-row">
-    <dl class="status-bar__section column-one-quarter">
+  <div class="l-container">


I think this class is causing some unnecessary spacing on smaller resolutions:

teneightfive · 2017-08-08T08:49:46Z

assets/stylesheets/components/_status-bar.scss

-      padding-left: ($gutter / 2) + (($gutter / 3) * 2);
-    }
-  }
-
  dt {


I wonder if we shouldn't be using element selectors here. I know it's not part of this change but might be worth addressing whilst you're looking at that component.

If we're sticking to BEM we should be trying to use classname selectors rather than element selectors.

feedmypixel · 2017-08-09T11:28:28Z

I have updated this component to use the c-meta-item styles as suggested by @tyom and @teneightfive

tyom · 2017-08-09T11:38:17Z

src/templates/_macros/investment-meta.njk

+    <dl>
+      {% for meta in props.meta %}
+        {% if meta.value %}
+          <div class="c-meta-item">


You can't have div as sibling of dl.

s/sibling/child

You can't have div as sibling of dl.

According to MDN its permitted content

It says there:

WHATWG HTML allows wrapping each name-value group in a <dl> element in a <div> element.

As far as I can tell there's still no consensus on this issue.

I wonder if the macro should deal with loops at all. The isLabelHidden is also a global setting and there seems to be no way of using that for a specific item.

What if there was a macro that creates a single key/value pair? And then maybe It could be used in a loop using map filter.

teneightfive · 2017-08-09T14:32:01Z

assets/stylesheets/components/_meta-item.scss

@@ -6,16 +6,20 @@
  + &:nth-child(even) {
    margin-left: $baseline-grid-unit;
  }
+
+  & + & {
+    margin-left: $default-spacing-unit;


This seems to affect it's current use on investment projects collection:

teneightfive · 2017-08-09T14:33:25Z

assets/stylesheets/components/_meta-item.scss

 }

 .c-meta-item__value {
-  color: $text-colour;
+  color: $grey-1;


This also changes how it previously looked on investment projects:

Before

After

teneightfive · 2017-08-09T14:36:33Z

src/templates/_macros/investment-meta.njk

+ # Render Meta fields for Investment
+ # @param {object[]} props.meta - Array of meta objects
+ #}
+{% macro InvestmentMeta (props) %}


This name feels very heavily tied to its content but I wonder if it needs to. The macro is just rendering a list of meta-item and it doesn't need to care if they're in investment or not.

This macro could be MetaData and added to _macros/common.njk (as much as I hate its generic name)?

feedmypixel · 2017-08-09T15:48:41Z

@tyom @teneightfive updates up for this

tyom · 2017-08-09T17:03:04Z

assets/stylesheets/components/_meta-bar.scss

+@import "../settings";
+
+.c-meta-bar {
+  margin-top: $default-spacing-unit;


This should be set in assemblies.

tyom · 2017-08-09T17:11:01Z

src/apps/investment-projects/views/_layout.njk

@@ -1,4 +1,5 @@
 {% extends "_layouts/two-column.njk" %}
+{% from "_macros/entities.njk" import MetaItem %}


Is this used here?

no - added here so its globally available

Maybe just import in datahub-layout.njk like all the other Macros. Then we don't have random imports scattered around. Also allows us to see which macros are available to the views at a glance.

ah my bad! this is meant to be in with all the others

tyom · 2017-08-09T17:15:04Z

src/apps/investment-projects/views/_layout.njk


-    {% if investmentData.archived %}
+  {{ Meta({


You could also do this without creating a new macro:

{{ [ { label: 'Project code', value: investmentStatus.projectCode }, { label: 'Valuation', value: investmentStatus.valuation } ] | map(callAsMacro('MetaItem')) | join | safe }}

Or set the data it in controller and just use one liner.

Over cornflakes I have been pondering this... The reason I didn't go with the MetaItem macro is because it is just using divs and it felt like there was a need for a dl here. Hence the creation of Meta component.

We could make it configurable like we do with other macros. e.g. pass el prop into macro.

tyom · 2017-08-09T17:16:15Z

src/templates/_macros/common.njk

+ #}
+{% macro Meta(props) %}
+  {% if props.meta | length %}
+    <dl class="c-meta-bar">


This component seems unique to one use case so far. Is it really a component? Could we avoid creating new macros for a single use case?

This component seems unique to one use case so far. Is it really a component? Could we avoid creating new macros for a single use case?

will do - ill add it in as described above

teneightfive · 2017-08-10T09:33:36Z

assets/stylesheets/components/_meta-list.scss

+@import "../settings";
+
+.c-meta-list {
+  margin-top: $default-spacing-unit;


What if it appears as the first element? Could use a sibling selector to tell it only to apply a top margin if it has a preceding element:

.c-meta-list { * + & { margin-top: $default-spacing-unit; } }

teneightfive · 2017-08-10T09:34:24Z

assets/stylesheets/components/_meta-list.scss

+  margin-top: $default-spacing-unit;
+
+  .c-meta-item {
+    margin-right: $default-spacing-unit;


Maybe use a sibling selector to set a left margin:

.c-meta-item { * + & { margin-left: $default-spacing-unit; } }

teneightfive · 2017-08-10T09:35:26Z

src/apps/investment-projects/views/_layout.njk


-    {% if investmentData.archived %}
+  {{ MetaList({
+    meta: [


teneightfive · 2017-08-10T09:38:52Z

src/templates/_macros/common.njk

+    <dl class="c-meta-list">
+      {% for meta in props.meta %}
+        {% if meta.value %}
+          <div class="c-meta-item">


Should we have a <div> as a child element of a <dl>?

I wonder if we should try to fit it into HTML semantics at this stage. We could use use non-semantic elements for now and refactor once we have a better idea how these are used across the apps (in groups or as individual elements).

Agreed 👍 we can align later when we know what this should be doing

feedmypixel added the multipart label Aug 7, 2017

ztolley temporarily deployed to datahub-beta2-pr-424 August 7, 2017 16:59 Inactive

feedmypixel force-pushed the feature/investment-project-stages branch 4 times, most recently from b65dd0c to 6f379b6 Compare August 7, 2017 19:32

feedmypixel force-pushed the feature/investment-status-updates branch from 5afaede to 95da091 Compare August 7, 2017 19:40

ztolley temporarily deployed to datahub-beta2-pr-424 August 7, 2017 19:40 Inactive

feedmypixel requested review from ztolley, tyom and teneightfive August 8, 2017 08:45

teneightfive suggested changes Aug 8, 2017

View reviewed changes

ztolley temporarily deployed to datahub-beta2-pr-424 August 9, 2017 11:27 Inactive

feedmypixel force-pushed the feature/investment-status-updates branch from 4cb4a43 to cf9c335 Compare August 9, 2017 11:30

ztolley temporarily deployed to datahub-beta2-pr-424 August 9, 2017 11:30 Inactive

tyom suggested changes Aug 9, 2017

View reviewed changes

feedmypixel force-pushed the feature/investment-project-stages branch from 6f379b6 to e7d260a Compare August 9, 2017 14:05

teneightfive suggested changes Aug 9, 2017

View reviewed changes

feedmypixel force-pushed the feature/investment-project-stages branch from e7d260a to 2d12ba8 Compare August 9, 2017 14:38

feedmypixel force-pushed the feature/investment-status-updates branch from cf9c335 to 3fd2686 Compare August 9, 2017 14:39

ztolley temporarily deployed to datahub-beta2-pr-424 August 9, 2017 14:39 Inactive

ztolley temporarily deployed to datahub-beta2-pr-424 August 9, 2017 15:40 Inactive

ztolley temporarily deployed to datahub-beta2-pr-424 August 9, 2017 15:47 Inactive

tyom reviewed Aug 9, 2017

View reviewed changes

feedmypixel force-pushed the feature/investment-project-stages branch from 6991049 to be30239 Compare August 9, 2017 21:26

feedmypixel force-pushed the feature/investment-status-updates branch from 459efea to 7db4867 Compare August 9, 2017 21:27

ztolley temporarily deployed to datahub-beta2-pr-424 August 9, 2017 21:27 Inactive

ztolley temporarily deployed to datahub-beta2-pr-424 August 10, 2017 06:13 Inactive

feedmypixel force-pushed the feature/investment-project-stages branch from be30239 to c7cf278 Compare August 10, 2017 08:38

feedmypixel force-pushed the feature/investment-status-updates branch from 1af823a to e853641 Compare August 10, 2017 08:40

ztolley temporarily deployed to datahub-beta2-pr-424 August 10, 2017 08:40 Inactive

feedmypixel force-pushed the feature/investment-project-stages branch from c7cf278 to 260c8a1 Compare August 10, 2017 09:08

feedmypixel force-pushed the feature/investment-status-updates branch from e853641 to 93b7cab Compare August 10, 2017 09:08

ztolley temporarily deployed to datahub-beta2-pr-424 August 10, 2017 09:08 Inactive

teneightfive reviewed Aug 10, 2017

View reviewed changes

src/apps/investment-projects/views/_layout.njk Outdated

{% if investmentData.archived %}

{{ MetaList({

meta: [

Copy link

Contributor

teneightfive Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

items?

teneightfive reviewed Aug 10, 2017

View reviewed changes

ztolley temporarily deployed to datahub-beta2-pr-424 August 10, 2017 13:08 Inactive

tyom approved these changes Aug 10, 2017

View reviewed changes

feedmypixel force-pushed the feature/investment-project-stages branch from 260c8a1 to 662195d Compare August 10, 2017 13:19

feedmypixel force-pushed the feature/investment-status-updates branch from d1238e8 to 3e22c0e Compare August 10, 2017 13:24

ztolley temporarily deployed to datahub-beta2-pr-424 August 10, 2017 13:25 Inactive

updates to investment status component

addf027

feedmypixel force-pushed the feature/investment-status-updates branch from 3e22c0e to addf027 Compare August 10, 2017 13:31

ztolley deployed to datahub-beta2-pr-424 August 10, 2017 13:31 View deployment

feedmypixel merged this pull request into feature/investment-project-stages Aug 10, 2017

teneightfive deleted the feature/investment-status-updates branch August 14, 2017 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates to investment status component #424

updates to investment status component #424

feedmypixel commented Aug 7, 2017 •

edited

Loading

tyom commented Aug 8, 2017

teneightfive Aug 8, 2017

teneightfive Aug 8, 2017

feedmypixel commented Aug 9, 2017 •

edited

Loading

tyom Aug 9, 2017

teneightfive Aug 9, 2017

feedmypixel Aug 9, 2017

tyom Aug 9, 2017

tyom Aug 9, 2017

teneightfive Aug 9, 2017

teneightfive Aug 9, 2017

teneightfive Aug 9, 2017

feedmypixel commented Aug 9, 2017

tyom Aug 9, 2017

tyom Aug 9, 2017

feedmypixel Aug 9, 2017

teneightfive Aug 10, 2017

feedmypixel Aug 10, 2017

tyom Aug 9, 2017

feedmypixel Aug 10, 2017

tyom Aug 10, 2017

tyom Aug 9, 2017

feedmypixel Aug 9, 2017

teneightfive Aug 10, 2017

teneightfive Aug 10, 2017

teneightfive Aug 10, 2017

teneightfive Aug 10, 2017

tyom Aug 10, 2017

feedmypixel Aug 10, 2017

		@@ -1,4 +1,5 @@
		{% extends "_layouts/two-column.njk" %}
		{% from "_macros/entities.njk" import MetaItem %}

updates to investment status component #424

updates to investment status component #424

Conversation

feedmypixel commented Aug 7, 2017 • edited Loading

tyom commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feedmypixel commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

feedmypixel commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feedmypixel commented Aug 7, 2017 •

edited

Loading

feedmypixel commented Aug 9, 2017 •

edited

Loading