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

Fix list view icons with custom links #2026

Merged
merged 2 commits into from May 20, 2018
Merged

Fix list view icons with custom links #2026

merged 2 commits into from May 20, 2018

Conversation

skurvish
Copy link

Created a new layout combining the custom links with the listicon-tips for the case of a custom link attached to an icon. See http://fabrikar.com/forums/index.php?threads/icon-link-in-list-to-custom-php-script.48785/ for details

@Sophist-UK
Copy link

At first sight the code looks good. Can you post before and after screen shots and HTML as before?

@skurvish
Copy link
Author

skurvish commented Apr 20, 2018

The screen shots are identical in either case, what is critical is the underlying html. Here is the html before the change:
image and here is the html after the change:
image
Note in the first image the 2 anchor elements inside the single table cell, versus the second with a single merged anchor.

@Sophist-UK
Copy link

Sophist-UK commented Apr 20, 2018

That looks cool. Not sure when Hugh will be able to merge it into the main Fabrik code.

@cheesegrits
Copy link
Member

It's merged in my local test branch, I'll merge it to master when I've finished testing.

@skurvish
Copy link
Author

skurvish commented May 1, 2018

Hugh, can we get this merged so I can move on with my project?

Copy link
Member

@cheesegrits cheesegrits left a comment

Choose a reason for hiding this comment

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

Use lower case for true and false

$a = $html->getElementsByTagName('a')->item(0);
$img = $html->getElementsByTagName('img')->item(0);
if (isset($html) && isset($a) && isset($img))
$isIcon = TRUE;
Copy link
Member

Choose a reason for hiding this comment

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

Put {} around the body. I know it's not technically required for a single statement.


$paths = array(
COM_FABRIK_FRONTEND . '/views/list/tmpl/' . $this->getTmpl() . '/layouts/element/' . $elementModel->getFullName(true, false)
);

$layout = $this->getLayout('element.fabrik-element-custom-link', $paths);
$layout = $isIcon ? $this->getLayout('element.fabrik-element-custom-icon-link', $paths) : $this->getLayout('element.fabrik-element-custom-link', $paths);
Copy link
Member

Choose a reason for hiding this comment

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

Avoid long lines, or PHP Storm will perpetually bitch at me about it. So only use inline condition for short stuff. Put this in a normal if/else.

@@ -2320,12 +2320,23 @@ public function _addLink($data, &$elementModel, $row, $repeatCounter = 0)
$rowId = $this->getSlug($row);
$isAjax = $this->isAjaxLinks() ? '1' : '0';
$isCustom = $customLink !== '';
$isIcon = FALSE;
Copy link
Member

Choose a reason for hiding this comment

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

lower case

$a = $html->getElementsByTagName('a')->item(0);
$img = $html->getElementsByTagName('img')->item(0);
if (isset($html) && isset($a) && isset($img))
$isIcon = TRUE;
Copy link
Member

Choose a reason for hiding this comment

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

lower case

@cheesegrits cheesegrits merged commit ed4174d into Fabrik:master May 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants