Conversation
robyww
added
bug
enhancement
multi-ticket
robyww
force-pushed
the
FIREFLY-1465-hips-panel
branch
from
09fbcbc to
bca04e6
Compare
loitly
left a comment
loitly
left a comment
There was a problem hiding this comment.
I added a couple of comments, but it's your call.
There one UI changes I would recommend, and it's easy to do.
Notice how there's a double border on the bottom and a bad round border on the top?
I suggest setting variant='plain' to the table, then add simple divider between the header and the table. This should make it looks more polished.
This one is my opinion. Can ignore if you disagree.
The uneven top-side borders of the images looks like it was done by mistake. I don't see the purpose. They are thicker than they need to be as well.
The bottom status bar is floating on top. If it's meant to be transparent so that more of the image is showing, I don't think it's doing the job. I only see it's off by a few pixels here or there, and it makes it look like the images are missing borders.
| String sourceUrl= request.getParam(HIPS_LIST_SOURCE); | ||
| String sourceUrlName= request.containsParam(HIPS_LIST_SOURCE_NAME) ? | ||
| request.getParam(HIPS_LIST_SOURCE_NAME) : sourceUrl; | ||
| if (sourceUrl != null && !sources.containsKey(sourceUrl)) { | ||
| sources.put(sourceUrlName, new URLHiPSListSource(sourceUrl,sourceUrl) ); | ||
| } | ||
| if (request.getBooleanParam(ENSURE_SOURCE)) { | ||
| return createDataGroupFromSources(); | ||
| } |
There was a problem hiding this comment.
The API for this processor seems unclear or tangled to me. It looks like you have a master(public) list of sources that can grow on demand but cannot shrink. Additionally, it handles both PUT and GET in one operation which made it more confusing.
ADD_HOC_SOURCE is poorly named. Maybe it meant to be ADHOC_SOURCES or ADD_ADHOC_SOURCES, but it is actually some sort of IVOA IDs that when given may affect the source as well as the filtering part of GET. From the name, if it's ADD, then I would expect to see it added to sources(master), but I don't see it.
With ENSURE_SOURCE added, you may PUT(add) one additional source into the list, but then return the latest list as is, ignore everything else(filtering, sorting, prioritization).
Keep in mind, Firefly has a multi-user backend. Imagine multiple notebook users loading(PUT) whatever they want, but now it is shared with or leaked into other sessions.
I don't have a suggestion; I'm just pointing out what I see.
There was a problem hiding this comment.
I can change ADD_HOC_SOURCE but you understand it has been like that for years. I will make a betters name (after I figure out what the parameters does), it is an easy change.
ENSURE_SOURCE is meant to be a different mode. I could have made a new processor but that seems like overkill. It is not a big deal to have many new sources names added since it does not search them until requested. I think they will support multi usage just fine. The request was for to be able to dynamically hit hips list servers. This approach is very low overhead. In the real world, multiple clients will call ENSURE_SOURCE with the same parameters so it will do nothing except return a table of sources.
Also a hips list servers is not meant to be secret and there will never be many. You would not be specifying this from a notebook. It would be via the firefly url api. If the approach needs to me more advanced we can update it in another round. However, right not it seems like a simpler approach is a good way to start.
I does not have to return anything. I will take that out. I mostly had it return the table of sources for debugging.
| current.saveOverflow= document.body.style.overflow; | ||
| document.body.style.overflow= 'hidden'; |
There was a problem hiding this comment.
Instead of saving and then resetting body.overflow, maybe we should not set it begin with.
There was a problem hiding this comment.
We had to set it when expanded to make the expand look correct. I don't remember but I think there was a scroll bar on some of the web pages that did not appear to do anything. This is just putting it back to what ever the web page had before it was expanded. I don't know of any other solution here.
There was a problem hiding this comment.
Ah. I see what you mean now. JoyUI is doing the same thing, except they are modifying the html element. Ignore this comment.
The bottom status bar is floating on top. If it's meant to be transparent so that more of the image is showing, I don't think it's doing the job. I only see it's off by a few pixels here or there, and it makes it look like the images are missing borders. I will tweek the placement of the mouse readout and a couple of pixels in grid mode and make the border 1px |
- Firefly-1329: full descriptions of schemas and tables in TAP query screen via tooltip - Fixed: expanded mode div in api sometime maintains size - incluees response to feedback
robyww
force-pushed
the
FIREFLY-1465-hips-panel
branch
from
c286453 to
0c0ba91
Compare
2 participants
Multiplie tickets
Testing