Skip to content

Added em tag#47

Merged
Stephanevg merged 2 commits into
Stephanevg:masterfrom
awickham10:master
Oct 4, 2018
Merged

Added em tag#47
Stephanevg merged 2 commits into
Stephanevg:masterfrom
awickham10:master

Conversation

@awickham10
Copy link
Copy Markdown
Contributor

Pull Request added em tag

Added em tag based on strong tag function.

Please tell us , the type of Change you are submitting:

Select one of the following:

  • Bug
  • Feature
  • Minor Change

Does it fix an existing issue? Please tell us which one

Contributes to issue #7.

Description of what's been changed

Added em.ps1 function and tests.

Results of your tests (If applicable)

image

Copy link
Copy Markdown
Owner

@Stephanevg Stephanevg left a comment

Choose a reason for hiding this comment

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

Hi @awickham10

Thanks a lot for your PR ! It is perfect!

Could ask you one very small change before merging it?

I noticed the help is still the one for the "

" element

<#
    .SYNOPSIS
    Generates em HTML tag.
     .Description
    This tag is a "Textual semantic" tag. To use it in a "P" tag, be sure to prefix it with a semicolon (";").
    See example for more details.
     .EXAMPLE
    p{
        "This is";em {"cool"}
    }
     Will generate the following code
     <p>
        This is
        <em>
        cool
        </em>
    </p>

You can find infos here to update it accordingly:

https://www.w3schools.com/tags/tag_em.asp

This should be done in not time.

Keep them comming 👍 !

@awickham10
Copy link
Copy Markdown
Contributor Author

Is this more what you were thinking?

    .SYNOPSIS
    Generates em HTML tag.

    .Description
    This tag is a phrase tag. It renders as emphasized text.

    .EXAMPLE
    p{
        "This is";em {"cool"}
    }

    Will generate the following code

    <p>
        This is
        <em>
        cool
        </em>
    </p>

@Stephanevg
Copy link
Copy Markdown
Owner

Yeah, that is great!
One very last thing, and then I'll merge: Could you change the $Content parameter from mandatory=$true to $false?

I think we should leave the possibility for the end user to set an em element with an empty value. (and modified using Javascript in a later step for example).
Thanks!

@awickham10
Copy link
Copy Markdown
Contributor Author

Done!

@Stephanevg Stephanevg merged commit 054ebe3 into Stephanevg:master Oct 4, 2018
@Stephanevg
Copy link
Copy Markdown
Owner

Awesome, Thanks a lot for your PR @awickham10 !

@Stephanevg Stephanevg mentioned this pull request Oct 4, 2018
100 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants