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

Refactor AddResourcesListener and Tooltips #347

Merged
merged 15 commits into from
Apr 28, 2016

Conversation

zhedar
Copy link
Collaborator

@zhedar zhedar commented Apr 17, 2016

The AddResourcesListener currently is a bit long and contains a lot of boilerplate code, therefore I made some refactorings. However, this is still WIP, so probably shouldn't be merged yet. For example addJavascript is still too long (220 lines for a single method) and the Comparator could be moved into an own class.

I just hoped to get some feedback (since it's getting late), e.g. on the difference between addResourceToHeadButAfterJQuery and addBasicJSResource. I couldn't spot any other than the different resource keys, but somehow I couldn't just get it to run with my extracted addResource method. The first seems to fail whereby the latter doesn't, so I just kept addResourceToHeadButAfterJQuery in place until I found the difference.

Changes:

  • made yes / no checks more readable and fixed case errors
  • extracted methods to reduce boilerplate code
  • reduced nesting levels
  • fixed typos

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 20, 2016

I just added another refactoring to this PR.
This one changes the way, how Tooltip resources are added (always add tooltip.css and tooltip.js in one method call).

However, in retrospect I saw that in some cases Tooltip.addResourceFile() wasn't called, although the tooltip.css file was added. Examples for this are Message, Badge or Messages. In other cases, only the js file was added, e.g. in CarouselControl or CarouselItem.
Is there a specific cause why these components just need one of the files or was it just forgotten to add the other one?

@zhedar zhedar changed the title Refactor AddResourcesListener Refactor AddResourcesListener and Tooltips Apr 20, 2016
@zhedar
Copy link
Collaborator Author

zhedar commented Apr 20, 2016

I just observed, that selectOneMenu's tooltip isn't shown, maybe the data attributes are not set properly. This problem is already present in the current snapshot showcase, so not a breaking change from my most recent changes. However, it isn't present in the 0.8.1 Showcase, so probably a bug we introduced in the Snapshot itself.
Edit: d48e9c2 kind of fixed the problem, the tooltip is now shown properly, but however isn't hidden, if the menu is clicked on until onblur happened, so another element is focused via another click. Maybe we could force-hide the tooltip on selecting an option, that may be a viable workaround.
Edit 2: OK, it actually behaves exactly as 0.8.1 did. That's still not the best result, but I guess we can temporarily live with that...

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 21, 2016

I just tried to add an ajax listener to remove the tooltips to fix #220 and #323, but somehow this got snowballed with every ajax request, if it was added in Tooltip.activeTooltips.
It seems like Tooltip.activeTooltips is called on any ajax request as well, so I just added a quick $('.tooltip').tooltip('destroy'); to its script.

While fixing this I discovered strange behaviour of b:poll, which polled just way too fast and saw #325 a lot again, I may investigate on that later on.

@zhedar zhedar mentioned this pull request Apr 21, 2016
@zhedar zhedar force-pushed the refactorings branch 3 times, most recently from f6787da to 49e4a74 Compare April 25, 2016 13:58
@zhedar
Copy link
Collaborator Author

zhedar commented Apr 25, 2016

The last commit I added(49e4a74) fixed the #325 related bug, which happened because PrimeFaces components seem to be duplicated.
The other warnings I saw came from the fact, that I used Bootstrap from CDN accidentally. We seem to remove every bsf related resource on such an occasion, which doesn't look like correct behaviour and should be fixed in the future.

I also solved the problem from the entry post and could replace two similar addResource methods by a generic one.

There's only one TODO left, before I consider this PR complete: There's still a nested if statement I marked as TODO in 49e4a74 I'd like to combine, but I'm not sure if that is actually correct as it stands, because it looks like the second statement always yields true if the first did as well.

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 27, 2016

I just cleared the last open point on my list, but I would like someone to review my latest change (e6e4e46). @stephanrauh maybe? The old statement looked kind of odd. It seems like it always evaluates to true anyways, what is this needed for? Do we even need those checks?

Otherwise I would declare this PR done... before it get's an big ugly abomination of changes I stitched together :O

@zhedar zhedar force-pushed the refactorings branch 3 times, most recently from 425021a to 178477a Compare April 28, 2016 00:26
@stephanrauh
Copy link
Collaborator

Whow! That's a huge one! I'm starting to test and merge this PR.

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 28, 2016

It would look smaller, if I wouldn't have named all the little changes I did and put them into separate commits though. I've tested most of this for two weeks now, so I'm rather confident that everything is going to be ok ;) Thanks for testing!

@stephanrauh stephanrauh merged commit 3427ac7 into TheCoder4eu:master Apr 28, 2016
@zhedar zhedar deleted the refactorings branch April 29, 2016 21:11
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