-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Investment project stages #395
Conversation
e3f28ce
to
56479d7
Compare
z-index: 20; | ||
} | ||
|
||
&.c-progress__bar--prospect-stage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be tempted not to use class names that relate to investment progress only. Maybe modifiers that relate to the percentage complete?
} | ||
} | ||
|
||
.c-progress__stage--active { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels more like a state class than a modifier. Maybe just is-active
.
left: $section-width * 4; | ||
} | ||
|
||
@include media($min-width: 1060px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does 1060px
relate to? Don't think we've used this breakpoint before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be worth looking at on smaller screens. Feels like the center aligned text for first and last items could get cut off at certain resolutions too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1060px
is when they can become centre aligned. The declaration on line 88 controls how they behave on smaller screens (pulling them in)
} | ||
|
||
.c-progress__stage--active { | ||
@include bold-font(16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the font size jump for current item. Think bold font weight might be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah was thinking about colourblind people and making it apparent enough that something has changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need more than colour change on the icon. Could be filled for current state and empty for other states?
Wonder if the investor company and code should be flipped to be consistent with investment projects pipeline layout. |
max-width: 4em; | ||
} | ||
|
||
.c-form-control--small { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should tweak the current .c-form-control--short
to be .c-form-control--long
, add .c-form-control--medium
and then set .c-form-control--short
to be 2.6em
?
Avoids us mixing short
and small
then.
max-width: 4em; | ||
} | ||
|
||
.c-form-control--small { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do
short
shorter
shortest
provide enough meaning when they are by themselves?
} | ||
} | ||
|
||
.status-bar__section-title { | ||
font-weight: 700; | ||
color: $grey-1; | ||
font-size: 16px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we're using core-font
mixin until we do a review of our typography.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enforced 👮 🚨
@@ -14,28 +13,18 @@ | |||
#} | |||
|
|||
{% if projectCode %} | |||
<div class="status-bar grid-row"> | |||
<dl class="status-bar__section column-one-quarter"> | |||
<div class="status-bar l-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we can reuse c-meta-item
here which is used on investment collection page and do away with status-bar
altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like a refactor/alignment for after this PR is in
{% from "_macros/form.njk" import Fieldset, TextField, MultipleChoiceField, HiddenField %} | ||
{% from "_macros/form.njk" import DateField %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge it with the line above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was wondering if we need to denote a difference between form elements and form patterns. The DateField felt more "patterny" (for want of a real word) than the others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need that kind of separation at this point. MultipleChoiceField
is also quite "patterny" but to the end user (developer) it just a component/macro with certain inputs.
src/templates/_macros/elements.njk
Outdated
# @param {string} props.text - Anchor text | ||
# @param {string} [props.target=_self] - Anchor target | ||
#} | ||
{% macro Link(props) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how useful this macro is. It's just an HTML element with a couple of attributes that are already restricted to certain values. I think you could justify having it if it had custom classes or wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed - the reason it was built is the having to send it in to the hint for the radios on the investment-type
page. I initially just sent in a html string with | safe
but it felt really hacky. This felt a little cleaner. Open to changing if your good with that.
src/templates/_macros/form.njk
Outdated
{% set buttonText = props.buttonText or 'Submit' %} | ||
|
||
{{ ErrorSummary(props.errors) }} | ||
<form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reuse existing Form
macro here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So differences from Form
:
- Error summary outside of form
- Button displayed inside form (not in a form group)
It felt like adding flags to the existing form to handle this was the wrong approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. But you're forking the form here so every time there are global changes to form we must remember to update this one as well. Perhaps we should add a flag to disable ErrorSummary
. You can already disable form submit button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should add a flag to disable ErrorSummary.
yeah sure could do it was more then wrapping of the submit button. But same could add a flag to not wrap it in c-form-group c-form-group--actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the InlineForm macro has now gone, after a rethink there is no need for this macro
{ | ||
value: form.options.investmentTypesObj.fdi.value, | ||
label: form.options.investmentTypesObj.fdi.label, | ||
hint: Link({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just save that link in a variable and pass it along to macro to avoid creating a new macro.
{% set investmentTypeFdiLink %}
<a href="/investment-projects/create/investment-type/info#fdi">Is this an FDI project?</a>
{% endset %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what is info
target? The target is meant to be a frame name. I think you meant _blank
to open in a new window. Maybe we can do an XHR to open inline? Perhaps a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target: 'info'
is to open in new window named info
so that it doesn't just open different blank pages when clicking different links
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just save that link in a variable and pass it along to macro to avoid creating a new macro.
This answers the above around creating a macro - will do
is there a component that can be shared with this? |
2fe7354
to
ab13e05
Compare
ab13e05
to
27be423
Compare
@tyom where is this? I can't track it down |
1e2264b
to
98473e7
Compare
260c8a1
to
662195d
Compare
e33a194
to
6e70288
Compare
2595104
to
92ffa00
Compare
label: 'Type of investment', | ||
value: form.state['investment_type'], | ||
error: form.errors.messages['investment_type'], | ||
children: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need children
. You can just call that macro and nest stuff in it. We should avoid multiple ways of doing the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need children.
do you mean use caller?
We should avoid multiple ways of doing the same thing.
This select
pattern is the same pattern used for radio
and checkbox
when using the MultipleChoiceField macro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will align this work to use caller
rather than children
src/templates/_macros/form.njk
Outdated
@@ -429,6 +438,10 @@ | |||
</option> | |||
{% endfor %} | |||
</select> | |||
|
|||
{% for child in fieldChildren %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is needed.
2b59a39
to
385ee23
Compare
385ee23
to
7401ccc
Compare
|
||
.c-meta-list { | ||
.c-meta-item { | ||
margin-right: $default-spacing-unit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A left margin when there is a sibling might be better here. But could be addressed later during a spacing review.
.c-meta-item + .c-meta-item {
margin-left: $default-spacing-unit;
}
added new form macros, improved url structure, added new controllers, views and routes, updated tests
… session under a namespace, provide accessors for this infromation
updates to global styles, display in /components, tests, added date pattern into investment-projects flow
tests, display in /components,
7401ccc
to
1c6470e
Compare
NOTE - the Investment Project flow itself is in discussion at present as we need some discovery to confirm we are meeting user needs. However the work here can be easily repurposed and is still valid
This work involves the first part of the investment creation project flow.
Of Note (see commits for more detailed information)
TODO
Create Investment project journey flow diagram
Investment project journey (uk and foreign)