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

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

Open
mkrecek234 opened this issue Jun 9, 2023 · 11 comments
Open

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

mkrecek234 opened this issue Jun 9, 2023 · 11 comments

Comments

@mkrecek234
Copy link
Contributor

mkrecek234 commented Jun 9, 2023

UPDATE: FUI issue: fomantic/Fomantic-UI#2972

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


  • You create a grid or crud $grid and set a model to it.
  • You add any tooltip decorator with
    $grid->addDecorator('name', new \Atk4\Ui\Table\Column\Tooltip(['tooltipField' => 'name', 'icon' => 'clock circle']));
  • You do $grid->addJsPaginatorInContainer(20, 500) to have it scroll and paginate inside a defined space

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 mvorisek changed the title Tooltip in grid with addJsPaginatorInContainer with wrong zIndex Tooltip in grid with addJsPaginatorInContainer has wrong z-index Jun 9, 2023
@mvorisek
Copy link
Member

mvorisek commented Jun 9, 2023

There are two issues.

One is maybe wrong z-index which should be fixed by implementing #1988 (comment) 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
Copy link
Member

mvorisek commented Jan 7, 2024

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

@mkrecek234
Copy link
Contributor Author

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
Copy link
Member

mvorisek commented Jan 7, 2024

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
Copy link
Contributor Author

mkrecek234 commented Jan 7, 2024

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
Copy link
Member

mvorisek commented Jan 7, 2024

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
Copy link
Contributor Author

mkrecek234 commented Jan 7, 2024

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
Copy link
Member

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.

@mvorisek mvorisek changed the title Tooltip in grid with addJsPaginatorInContainer has wrong z-index Replace table column popup from CSS only with FUI popup Jan 19, 2024
@mkrecek234
Copy link
Contributor Author

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
Copy link
Contributor Author

mkrecek234 commented Jan 19, 2024

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
Copy link
Member

$('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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants