Skip to content
This repository has been archived by the owner on Nov 27, 2020. It is now read-only.

Cleanup macros #134

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Cleanup macros #134

wants to merge 6 commits into from

Conversation

CalvinRodo
Copy link
Member

@CalvinRodo CalvinRodo commented Nov 26, 2019

Did some light cleanup work in the macros to remove some copy and pasted sections and pull them into their own macros should improve maintainability.

  • Created a hintText macro in hint-text.njk
    • renders hint text on control if attributes.hint is set
  • Created a labelText macro in label-text.njk
    • renders required if attributes.required is true
  • Moved if statement into validationMessage macro and renamed file to make it consistent with other controls.

I also moved macros that are only included in other macros into sub-macros folder (I can't think of a better name right now).

These could also be mashed up into a single file and import from there instead of having 3 imports in each file. However I've got some old baggage from my C# days about having only one thing per file.

Removed some copy pasted code from various macros
Renamed file to be consistent with other macros
Moved from base.njk as it wasn't being used outside of macros
Moved if statement into macro as it was always included with macro call
Explicitly add to each macro as an import
Macros that are only included in other macros are now in the sub-imports
folder
@timarney timarney temporarily deployed to cds-node-starter-pr-134 November 26, 2019 04:09 Inactive
@timarney
Copy link
Member

First off I like this idea as it cleans things up in general and makes it easier to modify from a single spot.

@jneen do you see any issues getting this merged into the work your doing in #132? I'm not spotting anything major at given a quick look.

Copy link
Member

@timarney timarney left a comment

Choose a reason for hiding this comment

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

Please add some notes in the Changelog under the latest (we haven't tagged that release yet)

@CalvinRodo
Copy link
Member Author

Changelog has been updated

timarney
timarney previously approved these changes Dec 2, 2019
@timarney
Copy link
Member

timarney commented Dec 2, 2019

@CalvinRodo feel free to merge.

@jneen
Copy link
Contributor

jneen commented Dec 2, 2019

Oy, okay, just seeing this now. It's going to be a bit of work to integrate this.

Copy link
Member

@timarney timarney left a comment

Choose a reason for hiding this comment

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

@jneen do you want to hold off on this?

@timarney
Copy link
Member

timarney commented Dec 2, 2019

Holding for @jneen to review.

@jneen
Copy link
Contributor

jneen commented Dec 2, 2019

I think that might be best - this seems easier to rebase against my changes. The only major thing that would have to change is the validationMessage macro, which currently seems to ignore its msg parameter. It should take just the relname parameter and use getError.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants