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

KS: UI > Listing > CharacteristicValue #2118

Merged

Conversation

bheyser
Copy link
Contributor

@bheyser bheyser commented Aug 21, 2019

This PR is a followup for the PR 2078. It considers the conceptional discussion with @Amstutz and @klees that tooks place at skype.

This PR introduces a new ui component called characteristic value listing:
UI > Listing > CharacteristicValue

The component is to be used to present listings of label characteristic value doubles that need to be presented side by side.

It is more about presenting longer labels for short characteristic values than a short term that is described with a longer description. This is why we need this characteristic value listing as a rival to the descriptive listing.

The component comes as a new component family containing the following leafs:
UI > Listing > CharacteristicValue > Text

This leaf is at the moment the only choice. It allows text/text value pairs giving the most flexibility at the moment. In the future there might be other kinds of leafs for e.g. float values that automatically align to the decimal separator.

Example Usage:

function base()
{
    global $DIC; /* @var \ILIAS\DI\Container $DIC */
    $f = $DIC->ui()->factory();
    $r = $DIC->ui()->renderer();

    $items = [
        'Any Label for the First Item' => 'Item 1',
        'Another Label for the Second Item' => 'Item 2',
        'Third Item Comes as Component' => 'Item 3',
        'Fourth Item Comes as Component' => 'Item 4'
    ];

    $listing = $f->listing()->characteristicValue()->text($items);

    return $r->render($listing);
}

image

The component is relevant for the following (allready scheduled) feature requests:

Continuous Testing Mode Usability Improvement
https://docu.ilias.de/goto_docu_wiki_wpage_5089_1357.html
image

Extended Test and Item Statistics
https://docu.ilias.de/goto_docu_wiki_wpage_3529_1357.html
image

Copy link
Contributor

@Amstutz Amstutz left a comment

Choose a reason for hiding this comment

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

Hi @bheyser

Thank you a lot for contributing to the UI Components.
The proposed interface LGTM. Some minor details bellow. I recommend the JF to accept this.

Please implement the following changes:

  • Improve Purpose desc: "Characteristic Value Listings are used to present characteristic values.“ is not very helpful. Expand to: „Characteristic value is understood here as a value to quantify or describe a state indicated by some key in listing“.
  • Rivals Description: Leave out, "It is to be used whenever the semantics of * a html descriptive listing is required that can be shown underneath."

Note that I will perform a review on the implementation as soon as the interface is accepted.

kindly,
@Amstutz

@bheyser
Copy link
Contributor Author

bheyser commented Aug 26, 2019

I changed the purpose and rival descriptions according to your suggestion @Amstutz ;-)

@matthiaskunkel
Copy link
Member

Jour Fixe, 26 AUG 2019 : We highly appreciate this PR and accept it for ILIAS 6. Please merge to trunk.

@Amstutz
Copy link
Contributor

Amstutz commented Aug 26, 2019

Hi @bhyeser

Here follows the promised review of the implementation.

Thx for providing tests and examples.

Please implement the following changes:

  • Strict Typing: use strictly typed params and return values wherever possible (e.g. public function render(Component $component, RendererInterface $default_renderer): string;
  • Padding Variable: Use @padding-small-horizontal: instead of 5px for the padding in your less.

As soon as done and fine with @klees, this can be merged IMO.

kindly,
@Amstutz

@Amstutz
Copy link
Contributor

Amstutz commented Sep 11, 2019

@bheyser , any update on this. Note that we intend to adopt the whole UI Compoents to PSR2 as soon as @klees is back on track. Merging will be difficult afterwards.

@bheyser
Copy link
Contributor Author

bheyser commented Sep 23, 2019

Again, I am very sorry for the delay -.- I now have added type hints wherever possible.
I also made use of less variables padding-small/large-horizontal/vertical for all my paddings.

@klees
Copy link
Member

klees commented Sep 23, 2019

Hi @bheyser, no problem. Please have a look into the tests. They fail... Thx!

@bheyser
Copy link
Contributor Author

bheyser commented Oct 7, 2019

UI Kitchensink currently seems to need Fully Qualified ans Absolute Classnames regarding Namspaces when used in return types (perhaps also affects php docs, i dont know), since I removed ALL ...

  • relative class names
  • alias class names
    ... the Travis CI Checks passes.

@klees klees merged commit e40880d into ILIAS-eLearning:trunk Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants