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 figures panel not showing when none of the figures has a paragraph. #223

Merged
merged 2 commits into from
Feb 5, 2021

Conversation

DavidMorenoCortina
Copy link

Figures panel doesn't show when the figures hasn't caption or the captions hasn't a paragraph. So when the panel is of "resource" type it should check if the treeView has nodes.

@gnott
Copy link
Member

gnott commented Jan 22, 2021

Re issue #221

@gnott
Copy link
Member

gnott commented Jan 29, 2021

The result is looking good when I test with eLife article 00639, which is an example in bug issue #206.

Thanks for preparing the PR here @DavidMorenoCortina!

I have a question I'll add an inline comment for.

@@ -136,6 +136,10 @@ Container.Prototype = function() {
return this.listView.length;
};

this.hasContent = function(panelType) {
Copy link
Member

Choose a reason for hiding this comment

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

@DavidMorenoCortina, I'm wondering in what situation panelType is used here, because if I remove it and change the code below, it gives the same effect for eLife articles. Is it a hook used elsewhere in other Lens implementations?

Copy link
Author

Choose a reason for hiding this comment

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

@gnott I used panelType in case other panel types are added and their content only have to be shown based on listView. At this moment that check is redundant because all panels are resource type so it could be simplified

this.hideToggle();
this.hide();
} else {
if (this.getContainer().hasContent(this.config.type)) {
Copy link
Author

Choose a reason for hiding this comment

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

@gnott I've added "this.config.type" to the PR, I missed it when did the fork, sorry.

Copy link
Member

Choose a reason for hiding this comment

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

That fills in the missing part, thanks for this @DavidMorenoCortina!

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.

2 participants