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.

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

SelectOption: Add method getLabel() and change method setLabel() #70

Merged

Conversation

sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Aug 24, 2022

This PR allows to easily get the label of the selected option. getContect() returns an array of ValidHtml[] and makes it harder to get the label as string.

Usage:

$form->getElement('user')->getOption($form->getValue('user'))->getLabel()

@nilmerg
Copy link
Member

nilmerg commented Sep 2, 2022

Please rebase here, there's a new test covering this change.

@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch 2 times, most recently from 72434e5 to 7fd8404 Compare September 2, 2022 11:07
@sukhwinder33445 sukhwinder33445 self-assigned this Sep 2, 2022
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

What is the purpose of this PR, i.e. why do you have to change the label after construction? Also, there's setContent() and getContent() which could just be used on the element.

@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch 4 times, most recently from e60b273 to b444565 Compare September 2, 2022 14:12
@sukhwinder33445
Copy link
Contributor Author

I noticed that label can be null in the constructor. It makes no sense to have an option without label. Maybe this should be fixed in a separate PR.

@lippserd
Copy link
Member

lippserd commented Sep 2, 2022

What is the purpose of this PR, i.e. why do you have to change the label after construction? Also, there's setContent() and getContent() which could just be used on the element.

So you have updated the description. I suppose now it's just a matter of getLabel(). What do you want do with the label?

@lippserd
Copy link
Member

lippserd commented Sep 2, 2022

I noticed that label can be null in the constructor. It makes no sense to have an option without label. Maybe this should be fixed in a separate PR.

We'll talk about that later. option can be used in several places and here and there it applies to have no value. For a select option, of course, this makes no sense. However, value could be empty, which then defaults to the content. I will create an issue for all this later. First, I want to understand what you want to do with the label.

@lippserd
Copy link
Member

lippserd commented Sep 2, 2022

@sukhwinder33445
Copy link
Contributor Author

What is the purpose of this PR, i.e. why do you have to change the label after construction? Also, there's setContent() and getContent() which could just be used on the element.

So you have updated the description. I suppose now it's just a matter of getLabel(). What do you want do with the label?

I have a use case for getLabel() in PartnerPortal.
I want to get the name of the customer (name = label, value = id) of the selected option. So $form->getElement('user')->getOption($form->getValue('user'))->getLabel() helps me to do that.

@lippserd
Copy link
Member

@sukhwinder33445 This can be closed, right?

@lippserd
Copy link
Member

@sukhwinder33445 This can be closed, right?

Talked with @nilmerg about this. I'll review the PR and we add it (maybe 😆).

Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

In addition to the requested changes, setLabel() must NOT be removed, see here for a usage example.

I would like to see the following tests:

  • testRendering
  • testRenderingAfterSetLabel
  • testGetLabel
  • testGetLabelAfterSetLabel

@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch 2 times, most recently from 4982037 to f2ec9ec Compare October 26, 2022 11:13
@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch from f2ec9ec to 440d09c Compare October 28, 2022 14:38
nilmerg
nilmerg previously approved these changes Nov 7, 2022
@nilmerg nilmerg added the enhancement New feature or request label Nov 8, 2022
@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch 2 times, most recently from ff6551f to 31f40ed Compare November 9, 2022 09:45
@nilmerg
Copy link
Member

nilmerg commented Nov 9, 2022

As we're changing this so fundamentally, may I ask what the purpose of a SelectOption without a value is? Once constructed, it's not possible to change it, so if not passed to the constructor, the value is permanently not set.
I'd also question that the label may not be set. What's the purpose of a SelectOption without a label?

@lippserd
Copy link
Member

lippserd commented Nov 9, 2022

As we're changing this so fundamentally, may I ask what the purpose of a SelectOption without a value is? Once constructed, it's not possible to change it, so if not passed to the constructor, the value is permanently not set. I'd also question that the label may not be set. What's the purpose of a SelectOption without a label?

null is already used as value: https://github.com/Icinga/icingaweb2-module-vspheredb/blob/master/library/Vspheredb/Web/Form/InfluxDbConnectionForm.php#L60

Not having a label is questionable though.

@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch from 31f40ed to 9985e4b Compare November 9, 2022 14:18
@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch from 9985e4b to 6583208 Compare November 9, 2022 14:22
@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch from 321bbf9 to 0860464 Compare November 29, 2022 15:00
@sukhwinder33445 sukhwinder33445 mentioned this pull request Nov 29, 2022
3 tasks
@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch from 0860464 to 560868a Compare November 30, 2022 08:52
nilmerg
nilmerg previously approved these changes Nov 30, 2022
@sukhwinder33445 sukhwinder33445 force-pushed the add-method-to-easy-access-label-of-select-elements-option branch 2 times, most recently from dd6fb7d to 3815410 Compare November 30, 2022 11:07
@nilmerg nilmerg merged commit 10ab957 into master Nov 30, 2022
@nilmerg nilmerg deleted the add-method-to-easy-access-label-of-select-elements-option branch November 30, 2022 13:12
@nilmerg nilmerg removed the request for review from lippserd November 30, 2022 13:12
TAINCER pushed a commit that referenced this pull request Dec 1, 2022
…70)

* SelectOption: Add method `getLabel()` and phpdoc

Easily get the label of the select element's value

* Introduce class `SelectOptionTest`

* SelectElementTest: Remove method `testLabelCanBeChanged()`

SelectOption now has its own test class
TAINCER pushed a commit that referenced this pull request Dec 13, 2022
…70)

* SelectOption: Add method `getLabel()` and phpdoc

Easily get the label of the select element's value

* Introduce class `SelectOptionTest`

* SelectElementTest: Remove method `testLabelCanBeChanged()`

SelectOption now has its own test class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants