atk4 / ui

Robust and easy to use PHP Framework for Web Apps
https://atk4-ui.readthedocs.io/en/stable/
MIT License
448 stars 106 forks source link

Replace table column popup from CSS only with FUI popup #2069

Open mkrecek234 opened 1 year ago

mkrecek234 commented 1 year ago

UPDATE: FUI issue: https://github.com/fomantic/Fomantic-UI/issues/2972

see discussion below, CSS only tooltip must be with FUI popup module


Result: The tooltip is underneath the record line above, as in this picture: image

Expected result: The tooltip should be rendered above all grid lines

mvorisek commented 1 year ago

There are two issues.

One is maybe wrong z-index which should be fixed by implementing https://github.com/atk4/ui/issues/1988#issuecomment-1546108276 first as the current scrolling solution is obsolete and hacky.

Another issue reported in https://discord.com/channels/657649422280425523/664569505950990382/1192786142299439124 is FUI tooltip might be cropped when it is inside a container with overflow set.

mvorisek commented 10 months ago

The CSS-only FUI tooltip must be replaced with FUI popup - https://fomantic-ui.com/modules/popup.html

mkrecek234 commented 10 months ago

Hi @mvorisek Thank you for your idea. in Table\Column\Tooltip::class I replaced the last statement in getDataCellHtml by this code to use the Javascript variant of the FUI popup, unfortunately this won't show up on hovering. Not sure why but may be a conflict with other ATK4 JS code? Observed in latest macOS Safari:

        return $this->getApp()->getTag('td', $attr, [
            ' {$' . $field->shortName . '}',
         ['i', ['class' => 'ui icon link {$_' . $field->shortName . '_icon} {$_' . $field->shortName . '_data_visible_class}',
                    'data-content' => '{$_' . $field->shortName . '_data_tooltip}']]
        ]);
mvorisek commented 10 months ago

Not sure what you mean, I belive you need much more code to do that - first generate different HTML, then JS, and then make sure the JS is correctly applied to table (also possibly partial) refreshes.

mkrecek234 commented 10 months ago

data-content uses the JS variant of the FUI popup, whereas data-tooltip the CSS variant. So this should at least implement what you suggest. Please see https://fomantic-ui.com/modules/popup.html

Sorry, I think I know what you mean - we have to initaite then also each popup in JS using

$('.activating.element')
  .popup()

Correct?

mvorisek commented 10 months ago

You need to handle partial table update - test on demos to verify with Crud and Crud with JS scolling, row remove, delete, update, paginator, ...

This is because table renders rows using template, we should sooner or later replace that with native render tree logic, then dynamic row rendering will be easier and then you code would be enough. Sponsors welcomed.

mkrecek234 commented 10 months ago

Here is a solution that is working smoothly transforming the tooltip into the JS variant and very simple to implement - in this example I changed it to the HTML multiline tooltip variant - let me know if anyone sees this as helpful to be merged in a PR:

    protected function init(): void
    {
        parent::init();
        $this->table->js(true, (new Jquery('.atk4popup'))->popup());

    }

    #[\Override]
    public function getDataCellHtml(Field $field = null, array $attr = []): string
    {
        if ($field === null) {
            throw new Exception('Tooltip can be used only with model field');
        }

        $bodyAttr = $this->getTagAttributes('body');

        $attr = array_merge_recursive($bodyAttr, $attr, ['class' => '{$_' . $field->shortName . '_tooltip}']);

        if (is_array($attr['class'] ?? null)) {
            $attr['class'] = implode(' ', $attr['class']);
        }

        return $this->getApp()->getTag('td', $attr, [
            ' {$' . $field->shortName . '}',
            ['i', ['class' => 'ui icon link atk4popup {$_' . $field->shortName . '_icon} {$_' . $field->shortName . '_data_visible_class}',
                'data-html' => '{$_' . $field->shortName . '_data_tooltip}',
                'data-variation' => 'multiline']]
        ]);
    }

There is no need to handle partial table updates in my opinion, deleted/ added rows work perfectly including the tooltips. Works perfect in JsPaginatorInContainer, and also in a JsScrolling setting.

mvorisek commented 10 months ago

The proposed solution can work as FUI allows FUI modules to be repeatably initialized, but with a long live updated table this will imply performance issue - see my previous posts, the elem.popup() JS must be called only once per **new** row element.

mkrecek234 commented 10 months ago

Hi @mvorisek, just a thought for discussion: Wouldn't it alternatively make sense to call the popup only ONCE in JQuery per App rendering as we have once CSS class only, independent if Tooltips exist or not? Not very elegant, but takes less resources than calling it multiple times. WDYT? Currently don't have an idea to code this as easy and effective as in this PR: Performance issues are only to be expected in extreme edge cases in my opinion.

mkrecek234 commented 10 months ago

P.S. Or we track if the popup was called once, and then omit in each rendering any further second, third,... call as those are not required. That would have been to be registered in the parent or App object though, right?

mvorisek commented 10 months ago

$('popup selector').popup() called only once per App will setup only the popups existing at the call time, but if more popups are added, they will be not setup. Simply said, it cannot work unless you use some magic using observers, which is probably no better than tracking this in atk4/ui code as I have written above.

I do not want to participate in this thread more, feel free to either propose a working and performing solution or accept what I said and what must be done.