refactor: move 'verticalAlign' object to 'view' object

[hamed-musallam]

Tasks

Preview Give feedback

1. move verticalAlign object to view object
2. refactor the vertical-align object and calculate the vertical shift on fly
3. refactor samples

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	bf3c2d1
Status:	✅  Deploy successful!
Preview URL:	https://553d6003.nmrium.pages.dev
Branch Preview URL:	https://refactor-alignment-property-.nmrium.pages.dev

View logs

[targos]

Shouldn't we have a more "generic" Record at this level with an object of properties? I expect more values to depend on the nucleus and having a separate record for each of them seems complicated.

[hamed-musallam]

Actually, we use it in different places:

  spectra: {
    /**
     * active spectrum id per nucleus
     * @default {}
     */
    activeSpectra: Record<Nucleus , ActiveSpectrum[] | null>;
    },
verticalAlign: Partial<Record<Nuclei, VerticalAlignment>>

but I think that it is more precise and specific to have it like that. for example, verticalAlign is only for 1d so it does not make sense to have this value for a 2d nucleus

 { "1H,1H" :{
     verticalAlign,
     activeSpectra
     }
   }

[hamed-musallam]

@targos

What do you think ?

[targos]

Then what about an object for all 1D-specific parameters which depend on the nucleus?

[hamed-musallam]

@targos

Yes, it could be like that, let's assume we have

view:{
 1H: {
   verticalAlign,
 activeSpectrum,
    ...etc
  },

},

in above structure how I can force specific type for the object based on the Nucleus ?

Or

view1D:{
    1H: {
       verticalAlign,
     activeSpectrum,
        ...etc
      },
}

view2D:{
  "1H,1H":{
     activeSpectrum,
     ...etc
    }
}

on the other hand, we will consider that all object is not optional so we should initiate the object with an initial value, which could be null . This happens on the first time, we attend to store a value, for example, the active spectrum and the object 1H object does not exist.

{
  1H: { 
         verticalAlign: "bottom",
          activeSpectrum:  1,
          x:null,
          y: null,
         ...etc
          }
}

[targos]

I think we should avoid mixing 1D and 2D when they have different interfaces, so my preference goes towards view1D/view2D.

[hamed-musallam]

Ok, then we should have 3 objects

view: {
// which contains all objects that are not grouped by the nucleus.
 },
view1D,
view2D

What do think about the other object that is per spectrum, for example,

{
  ranges: Array<RangeToolState>;
  zones: Array<ZoneToolState>;
  /**
   * peaks view property
   * where the key is the id of the spectrum
   */
  peaks: Record<string, PeaksViewState>;   
  zoom: {
    levels: ContoursLevels; // for 2D only
  }
}

[hamed-musallam]

What do you think about this structure ?

view: {
  nuclei:{
    "1D":{
      "1H":{
         verticalAlign: "bottom",
         activeSpectra:"",
         selectReferences:null,
        }
     },
     "2D":{
     "1H,1H":{
         activeSpectra:"",
         selectReferences:null,
        }
       }
   },
  spectra: {
    /**
     * current select tab (nucleus)
     * @default null
     */
    activeTab: string;

    /**
     * show the spectra legend and the intensity
     * @default false
     */
    showLegend: boolean;
   }
}

[targos]

Ok, then we should have 3 objects

No, we should have everything inside of view:

view: {
  nuclei1D: { // Record<string, ViewNuclei1D>
    '1H': {},
    '13C': {},
  },
  nuclei2D: { // Record<string, ViewNuclei2D>
    '1H,1H': {},
  },
  spectra1D: { // Record<string, ViewSpectra1D>
    uuid1: {},
    uuid2: {},
  },
  spectra2D: { // Record<string, ViewSpectra2D>
    uuid3: {},
    uuid4: {},
  },
  prop1, // other settings that don't depend on nucleus or spectrum
  prop2,
}

[hamed-musallam]

@targos thanks, we need to consider this refactoring before having a new release of NMRium otherwise we have to write a migration script