Feature/pinning #25
Feature/pinning #25
Conversation
|
Re the color scheme, since it doesn't make sense to have an unlimited number of panels, we can just take one of the default plotly palettes and use it. Those are already designed to be contrasting, and using a dynamically generated palette is really overkill. |
|
I’ve handled this by removing them from the root but keeping their data stored
… On Jun 5, 2020, at 11:41 AM, Guillaume Fraux ***@***.***> wrote:
@Luthaf commented on this pull request.
In src/map/map.ts <#25 (comment)>:
> @@ -876,7 +913,11 @@ export class PropertiesMap {
*/
private _xValues(trace?: number): number[][] {
const values = this._properties()[this._current.x].values;
- const selected = [values[this._selected]];
+ const selected = [];
+ for (const guid of this._guids) {
+ selected.push(values[this._markerMap[guid].current]);
+ }
+
if (!this._is3D()) {
Just realised this needs to be adapted with the code above, setting all selected markers positions to NaN in 2D mode (and not only the first one), since in 2D mode we are manually dealing with the markers instead of leaving it to plotly for performance reasons.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#25 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALKVP3W5UTOOMPEI7Y57A4LRVC4UBANCNFSM4NOG5EXQ>.
|
|
A few other things to be done:
|
| this.info.show(indexes); | ||
| this.structure.onselect = (indexes, selectedGUID) => { | ||
| this.map.select(indexes, selectedGUID); | ||
| if (indexes.structure > 0 && indexes.environment > 0) { |
There was a problem hiding this comment.
Is this check needed because you pass -1, -1 to indicate removal of one viewer? If so, I think the code would be clearer by having a separate onremove callback. This can be refactored later.
src/map/map.ts
Outdated
| constructor(id: string, | ||
| indexer: EnvironmentIndexer, | ||
| properties: {[name: string]: Property}, | ||
| starterGuid: string, /// fix me later |
There was a problem hiding this comment.
fix me? This should also be documented with @param XXX, especially since it is part of the public interface
There was a problem hiding this comment.
Also, this should at least be made optional, since the map should still be usable without a structure viewer to provide the GUIDs. In this case, we may want to support a map without a "selected" marker
This can be refactored later
src/utils/index.ts
Outdated
| 'Maroon', 'Olive', 'Silver', 'Lime', | ||
| 'Navy', 'Fuchsia']; | ||
|
|
||
| // to be replaced later |
src/map/map.ts
Outdated
| const oldButton = document.getElementById(`chsp-selected-${this._active}`); | ||
| if (oldButton !== null) { | ||
| oldButton.classList.toggle('chsp-inactive-structure-marker', true); | ||
| oldButton.classList.toggle('chsp-active-structure-marker', false); |
There was a problem hiding this comment.
Instead of toggling classes inside the map code, it might be worth it to extract the 2D marker to its own class, with functions like move, makeActive, makeInactive, remove, etc.
this can be refactored later
src/structure/structure.ts
Outdated
| oldButton.innerHTML = `<span class="tooltiptext">Choose as active.</span>`; | ||
| } | ||
| this._active = activeGUID; | ||
| const newButton = getByID(`chsp-activate-${this._active}`); |
There was a problem hiding this comment.
Same as in the map, the class toggling logic could be extracted to a separate GridCell class, with appropriate functions.
This can be refactored later
|
100! |
|
Remaining TODO items:
|
|
The last few changes seem to fix the Safari issue, now there is only the Azaphenacenes one to fix |
|
The Azaphenacenes/on demand loading issue was not too hard to fix, and I also found & fixed an issue with initial setting placement. I've deployed a new beta at http://luthaf.fr/chemiscope/beta/pinning/, please play with it and try to break it! Except for bugs found in the beta, this should be good to go! |
Is this error related to those in beta? I got it from switching from Arginine to CSD. (It doesn't occur with switching to any other dataset) |
By "bug found in beta" I mean bugs that will be found in beta, this is one of them! The error is weird, looks like a malformed input JSON. I have a slightly different error, I'll look into this if no-one beats me! |
|
New blocker issue:
|
I thought we didn't want them* in 3D? Assuming the |
|
Yeah, I am talking about the 'selected' markers. Why would we not want them? We had one marker before, I would rather keep it =) |
Can we make this part of a future PR? Whenever I didn't code them out it would freeze the 3D map |
I think they should be there, I also noticed they were missing, and it's a major regression as one can't see who is who, breaking the viewer/map relationship |
|
Badass bug: whenever the 2D (but not the 3D!!) plot is redrawn, the last structure in the grid becomes the active one. Even just zooming in or out will transfer the focus to the last structure. |
This seems related to the update_AllMarkers() call on line 1295. which incidentally I guess has to do with the fact that highlighted symbols do not automatically scale when mousewheel-zooming. |
I know what this is from -- when switching from 3D to 2D, the map adds all of the markers back, and the |
|
Was actually a separate bug in which just zooming in 2D would switch the active point. I pushed a fix but it's ugly, the problem is that updateMarkers function also changes the active point. |
And simplify CSS related to selected structure markers. In particular even if the marker is not for the currently active one (i.e. is smaller) it will still stay centered on the corresponding point of the map
This allow "on demand loading" of structures
And unify interface by directly setting the callback as a property of the PropertyMap/StructureViewerGrid/JSmolWidget
|
Ok, this looks good to me. I rebased & force pushed since we had duplicated commits. If you want to check it out locally, make sure to use |
The code no longer tries to load a structure in the constructor
…ctive. Had to do it somewhat hackily because the only properties available in the click event for comparison require iteration through all selected points. Will become slow if there are a lot of selected points.
…writing the set active function within structure.ts, which had been rewritten to streamline the code, but missed a few nuances: - structure.ts: - whenever you set the active button, it puts a whole domino effect in motion. Therefore we should avoid doing `this._active = guid` as it will ignore this domino effect. - when the grid is switching datasets/setting up anew/etc. there will be no `old` active - if the grid tries to select an active which doesn't exist on the grid, do nothing - there is now a catch to check if the active is already equal to the argument passed - map.ts - I've moved the onselect exclusively into select, so that it doesn't get triggered with marker aesthetics updates (as it previously was in updateSelectedMarker) - when going from 2D->3D and vv., save the current active and restore it after switching. With the previous note it should not be necessary, but it's a failsafe just in case
#16
This is a feature addition of the widget "grid" to the right side of the chemiscope, allowing for viewing of multiple widgets at once. The grid allows for anywhere from 1-12 widgets.
There are three new buttons on each widget instance, an "active" flag, a "duplicate" button, and a "close" button:
Overview of code changes:
overarching change:
onselectnow takes indexes and a guid, so that the map/info/grid know which guid to operate on/set active.src/index.tshas been adapted for proper communicationstatic/chemiscope.csscontains the css objects for all buttonsstructure/structure.ts(large changes): now contains a grid instance rather than a single widget. Contains all infrastructure for adding/removing widgets, including the_widgetMap(Map): mapping from guids to widget, color, and current structure_guids(string[], used for indexing): guids of the widgets in the grid_active(string): guid of the active widgetbestArrangement(number): hacky function for determining the best arrangement of the widget gridget active(): returns active guidset active()calls _setActive_setActive(string): sets active guid_resizeGridInc(number): resizes the grid by a given increment_setupCell(cell#, row#, column#): sets up grid cell_setupGrid(): sets up the grid_removeWidget: kills the given widgetstructure/widget.ts(minimal changes): allows for optional passing of a guid for the widget. This allows for keeping the same size graph upon changing the dataset. Not critical, and can be changed if needed later on.map/map.ts(medium changes), includingnew variables:
_active (string): guid of the active widget_markerMap (Map): mapping from guids to color, current structure and marker_guids (string[], used for indexing): guids of the widgets in the gridnew functions
set active()calls _setActive_setActive(string): sets active guid_removeMarker(guid): kills the given markerand changes to existing functions
constructor(),select(), andupdateSelectedMarker()take a guid to synchronize with widgetsRequested Changes:
Outstanding Issues: