Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Small fixes to PodeWebTextbox #251

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Small fixes to PodeWebTextbox #251

wants to merge 12 commits into from

Conversation

ittchmh
Copy link

@ittchmh ittchmh commented Dec 20, 2021

Description of the Change

Updates:

  • function Update-PodeWebTextbox: update rows property
  • default.js: form stepper ignore enter key press for multi line textbox, update rows property
  • textbox.pode: set textbox value

Badgerati and others added 11 commits February 5, 2021 19:05
@Badgerati
Merge pull request Badgerati#26 from Badgerati/develop
ab8f83b 
v0.3.0
@Badgerati
Merge pull request Badgerati#42 from Badgerati/develop
c94e02f 
v0.4.0
@Badgerati
Merge pull request Badgerati#58 from Badgerati/develop
80b554f 
v0.4.1
@Badgerati
Merge pull request Badgerati#112 from Badgerati/develop
212555c 
v0.5.0
@Badgerati
Merge pull request Badgerati#127 from Badgerati/develop
5fe11ea 
v0.5.1
@Badgerati
Merge pull request Badgerati#180 from Badgerati/develop
d9a0671 
v0.6.0
@Badgerati
Merge pull request Badgerati#193 from Badgerati/develop
263b0be 
v0.6.1
@Badgerati
Merge pull request Badgerati#206 from Badgerati/develop
d877325 
v0.6.2
@Badgerati
Merge pull request Badgerati#248 from Badgerati/develop
57f8559 
v0.7.0
@ittchmh
Updates:
112ed83 
 - function Update-PodeWebTextbox: update rows property
 - function Set-PodeWebHomePage: enable IIS auth
 - default.js:  form stepper ignore enter key press for multi line textbox, update rows property
 - textbox.pode: set textbox value
@ittchmh
Merge pull request #1 from ittchmh/PR57f85591
8fdfa89 
PR57f85591
@ittchmh ittchmh changed the base branch from develop to master December 20, 2021 09:44
@ittchmh ittchmh changed the title Master Small fixes to PodeWebTextbox Dec 20, 2021
@Badgerati
Copy link
Owner

Hey @ittchmh,

I should be able to review this tomorrow, one quick thing though, could you update the PR to point at the develop branch? :)

Badgerati
Badgerati requested changes Dec 22, 2021
View reviewed changes
src/Templates/Views/elements/textbox.pode Outdated
@@ -31,7 +31,7 @@
$events = ConvertTo-PodeWebEvents -Events $data.Events

if ($data.Multiline) {
$element = "<textarea class='form-control $(if ($data.NoForm) { 'no-form' })' id='$($data.ID)' name='$($data.Name)' pode-object='$($data.ObjectType)' placeholder='$($data.Placeholder)' rows='$($data.Size)' style='$($width) $($data.CssStyles)' $($describedBy) $($readOnly) $($required) $($value) $($events)></textarea>"
$element = "<textarea class='form-control $(if ($data.NoForm) { 'no-form' })' id='$($data.ID)' name='$($data.Name)' pode-object='$($data.ObjectType)' placeholder='$($data.Placeholder)' rows='$($data.Size)' style='$($width) $($data.CssStyles)' $($describedBy) $($readOnly) $($required) $($value) $($events)>$($data.Value)</textarea>"
Copy link
Owner

Choose a reason for hiding this comment

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

Probably worth removing the $($value) property being set here.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/Public/Outputs.ps1 Outdated
Comment on lines 517 to 519
[Alias('Height')]
[int]
$Size = 4,
Copy link
Owner

Choose a reason for hiding this comment

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

I'd make it so the default here is 0, so we can skip updating the textarea if no Size is passed.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/Public/Outputs.ps1 Outdated
Comment on lines 543 to 545
if ($Size -le 0) {
$Size = 4
}
Copy link
Owner

Choose a reason for hiding this comment

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

Can remove this part if default size is 0.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/Templates/Public/scripts/default.js Outdated
Comment on lines 2798 to 2800
if (Number.isInteger(action.Size)) {
txt[0].rows = action.Size;
}
Copy link
Owner

Choose a reason for hiding this comment

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

The Size parameter is an [int] so the Number check isn't needed. It was also be worth checking that the textbox is a textarea as well, something like:

if (action.Multiline && action.Size > 0) {
    txt[0].rows = action.Size;
}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/Templates/Public/scripts/default.js
@@ -409,7 +409,7 @@ function setupSteppers() {
btn = stepper.find('.bs-stepper-content .bs-stepper-pane.active button.step-submit');
}

if (btn) {
if (btn && !isEnterKey(e)) {
Copy link
Owner

Choose a reason for hiding this comment

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

is the !isEnterKey(e) needed here? As ~9 lines up we do the same check and return 🤔

Copy link
Author

@ittchmh ittchmh Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If the && !isEnterKey(e) not set, it adding new line and execute Next button actio
7rAOMmPD46
n

@ittchmh ittchmh changed the base branch from master to develop December 22, 2021 18:05
ittchmh added a commit to ittchmh/Pode.Web that referenced this pull request Dec 22, 2021
@ittchmh
PR Badgerati#251 Fixes
75f2f4c
ittchmh added a commit to ittchmh/Pode.Web that referenced this pull request Dec 22, 2021
@ittchmh
PR Badgerati#251 Fixes
d7841b5
@ittchmh
Copy link
Author

ittchmh commented Dec 22, 2021

Hey @Badgerati, I tried to switch to develop brunch and revert master, but something was done wrong by me. 😕
Maybe better to clean up my forked repo and create new PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Badgerati Badgerati Badgerati requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@ittchmh ittchmh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ittchmh @Badgerati