Closed futuremotiondev closed 9 months ago
Nice! I've added a couple of review comments. When you're happy with it I'll hit merge thanks.
I would consider adding a GitHub issue to track the ideas you had for the unused config and mentioning it in a comment in the config file so we don't forget about it
Nice! I've added a couple of review comments. When you're happy with it I'll hit merge thanks.
Where can I see the review comments? Sorry for my inexperience. This is the first project I've actually contributed to so I'm not as well versed with GitHub as I'd like to be.
I would consider adding a GitHub issue to track the ideas you had for the unused config and mentioning it in a comment in the config file so we don't forget about it
Will do. I'll create an issue shortly.
One thing, when you say "mentioning it in a comment in the config file", do you mean just to add standard powershell comments in the actual config file itself (Initialize-DefaultColors.ps1
)?
Thanks for the help.
Nice! I've added a couple of review comments. When you're happy with it I'll hit merge thanks.
Where can I see the review comments? Sorry for my inexperience. This is the first project I've actually contributed to so I'm not as well versed with GitHub as I'd like to be.
they should be visible in the list here, guessing something went wrong and they were not saved.
looks cool.
Do we really need a nested Add-Column? i think it would be better to either integrate into the existing function or break out into its own private function, it would allow for testing etc.
to support a more flexible theming i think it would be easier to do something like this, a bit of a quick mock up..
@'
@{
AccentColor = 'Blue'
DefaultValueColor = 'Grey'
HeaderColor = 'Green'
PromptColor = 'yellow'
}
'@ | Set-Content .\test.psd1
function Import-SpectreTheme {
param(
[Parameter(Mandatory)]
[ValidateNotNullOrEmpty()]
[string]$ThemeName
)
<#
another option would be to make the theme files with actual `[Spectre.Console.Color]::` and just do
$script:theme = Import-PowerShellDataFile -Path "Name.psd1"
#>
$Import = Import-PowerShellDataFile -Path ".\$ThemeName.psd1"
$script:theme = @{}
foreach ($kvp in $Import.GetEnumerator()) {
$theme[$kvp.Key] = [Spectre.Console.Color]::$($kvp.Value)
}
}
Import-SpectreTheme -ThemeName test
and then using like $theme.HeaderColor
in the parameter block
i dont actually think .ToMarkUp()
is needed that just makes it a string which we would need to convert again, Convert-ToSpectreColor
will recognize its an actual color object type and return it properly.
oh bit of a caveat here that i have not had a chance to test the code so might need some tweaks.
feel free to pm me on discord if you want to continue our earlier discussion.
i realized my point above with .ToMarkup()
can have some unintended consequences..
We could implement an ArgumentTransformationAttribute class
This would be instead of the validator used today.
using namespace System.Management.Automation
using namespace Spectre.Console
class ColorTransformationAttribute : ArgumentTransformationAttribute {
[object] Transform([EngineIntrinsics]$engine,[object]$inputData) {
if ($InputData -is [Spectre.Console.Color]) {
return $InputData
}
if ($InputData.StartsWith('#')) {
$hexString = $InputData -replace '^#', ''
$hexBytes = [System.Convert]::FromHexString($hexString)
return [Spectre.Console.Color]::new($hexBytes[0], $hexBytes[1], $hexBytes[2])
}
if ($InputData -is [String]) {
return [Spectre.Console.Color]::$InputData
}
else {
throw [System.ArgumentException]::new("Cannot convert '$InputData' to [Spectre.Console.Color]")
}
}
}
# just adding this here so it's complete sample you can test. it's unchanged from the completers file.
class ArgumentCompletionsSpectreColors : System.Management.Automation.ArgumentCompleterAttribute {
ArgumentCompletionsSpectreColors() : base({
param($commandName, $parameterName, $wordToComplete, $commandAst, $fakeBoundParameters)
$options = [Spectre.Console.Color] | Get-Member -Static -Type Properties | Select-Object -ExpandProperty Name
return $options | Where-Object { $_ -like "$wordToComplete*" }
}) { }
}
function ColorTest {
param(
[ColorTransformationAttribute()]
[ArgumentCompletionsSpectreColors()]
[Spectre.Console.Color]$Color
)
return $Color
}
ColorTest -Color '#FFFFFF'
ColorTest -Color White
ColorTest -Color ([Spectre.Console.Color]::DarkOliveGreen3_1)
transformation would ensure that we always have a color object or it would throw.
It would also remove the need for Convert-ToSpectreColor
I think my GitHub app timed out it never submitted the review sorry I've been on flaky internet while travelling 😅
The comments should be there now in the files to review tab.
Add-TableColumns
to support a new table header color parameter.Format-SpectreTable
to support the new table header color parameter.psm1
to reflect this change.Now all table headers can be customized as such:
Lastly, I didn't have the time, but I added a new color default called
$script:DefaultTableTextColor
alongside of the$script:DefaultTableHeaderColor
.$script:DefaultTableTextColor
does nothing at the moment as it hasn't been implemented.$script:DefaultTableHeaderColor
does work now though.Let me know any thoughts on the PR!