fix-domManipulator

[KobyFowler]

Is your feature request related to a problem? Please describe.

Violation of SRP and tight coupling that could be solved.

Describe the solution you'd like

Introduces a separation of concerns by extracting DOM manipulation logic into a dedicated DomManipulator class. This approach centralizes the responsibility for managing selection-related DOM attributes, making the code more modular and easier to maintain. The DomManipulator class provides methods for adding and removing selection attributes, using classList for better performance and flexibility. The Selection class is then modified to use an instance of DomManipulator, delegating DOM operations to this specialized class. This refactoring improves the structure by isolating DOM interactions, potentially making the code easier to test and modify in the future. It also provides a clear interface for selection-related DOM operations, which could be extended or modified more easily as requirements change.

Describe alternatives you've considered

We could create a SelectableDecorator that wraps the selectable objects and adds methods for managing their selection state. This decorator would encapsulate the DOM manipulation logic within each selectable object, allowing for more flexibility and potentially easier testing of individual components. This approach would keep the selection logic close to the objects being selected, which could be beneficial if different types of selectables need different selection behaviors in the future.

Additional context

Code change will occur in src/selection/Selection.js

Code solution provided by @WavesJ99

[WavesJ99]

I endorse this issue!

[KobyFowler]

// In Selection.js
// Before Refactor
addSelectionAttributes(selectable) {
  if (selectable[0] && selectable[0].element) {
    selectable[0].element.setAttribute('s-selected', '');
  }
  if (selectable[1] && selectable[1].element) {
    selectable[1].element.setAttribute('s-selected-parent', '');
  }
}

// After Refactor
class DomManipulator {
  addSelectionAttributes(selectable) {
    if (selectable[0] && selectable[0].element) {
      selectable[0].element.classList.add('selected');
    }
    if (selectable[1] && selectable[1].element) {
      selectable[1].element.classList.add('selected-parent');
    }
  }

  removeSelectionAttributes(selectable) {
    if (selectable[0] && selectable[0].element) {
      selectable[0].element.classList.remove('selected');
    }
    if (selectable[1] && selectable[1].element) {
      selectable[1].element.classList.remove('selected-parent');
    }
  }
}

// Step 2: Modify Selection.js to Use DomManipulator:
class Selection {
  constructor(openmct, domManipulator) {
    this.openmct = openmct;
    this.domManipulator = domManipulator;
  }

  // Instead of handling DOM directly
  select(selectable) {
    this.domManipulator.addSelectionAttributes(selectable);
    // Other selection logic
  }

  deselect(selectable) {
    this.domManipulator.removeSelectionAttributes(selectable);
    // Other deselection logic
  }
}

[ozyx]

@KobyFowler @WavesJ99,

Thank you both for the suggestion! I have a few follow-up questions and comments:

1. You mentioned that switching from attributes to classes could improve performance. From my understanding, the performance gains in this case would likely be minimal, given how infrequently this code is executed. Could you provide more insight into your reasoning behind this suggestion?
2. It seems that the DomManipulator is still tightly coupled with the Selection class, as it contains logic specific to Selection. Could you clarify how this proposal would address that coupling?
3. Lastly, could you elaborate on the specific issues this approach aims to resolve beyond improving SRP (Single Responsibility Principle) and reducing coupling?

Looking forward to your thoughts!

[WavesJ99]

Hey Jesse! @ozyx I hope to answer your questions:

1. My reasoning behind this suggestion while it is small it is something that I was looking for. I am on a team with a few other classmates and we were assigned to make a small contribution to this project. The reason for this small contribution is not to make any big changes but rather get us used to the community and how to make a pr.
2. I thought that this would fix the tight coupling but it appears not :( any advice to help us out?
3. The fix is aimed to be a small fix that just improves the software architecture. If you have any more question or advice please let me know!

[ozyx]

Thanks for the info @WavesJ99 !

Sounds like a cool class. Is there a requirement to get your changes merged into the codebase? If so, what is your timeline on when you need to get this done? I ask because to contribute to this repository you'll need to submit a CLA. The CLA approval process can take a variable amount of time, sometimes quite a long time.

If that's not an issue, I can help you find something in our backlog that you could contribute which meets the criteria. That way you'll not only be completing your project requirements, but you'll be directly helping a NASA project!

Let me know what you think and if it's feasible given the CLA timeline and requirements.

[WavesJ99]

Thanks for the response @ozyx

We aren't required to have the changes merged just to create our first PR. I also ensured my whole team submitted the CLA, but we understand that may take some time. It would be amazing if you could point us in the direction of something in the backlog that needs to be worked on. The focus of the class is Software architecture so I believe we have to try and find an improvement we can make within the architecture.

We are working our way through a new version of updating this class to continue the first PR for the assignments sake but would love if you could point us in the direction of other work that might be better for us to work on.

Again thank you for the responses we are more than excited get in contact with you and start contributing to the repo.

[ozyx]

@WavesJ99

We aren't required to have the changes merged just to create our first PR.

In that case, why don't you and your team continue on the same path and I can help guide you? Making an architectural change to such a long-standing app won't be easy, but at the very least you can gain some experience contributing to the open source and interacting with open source maintainers.

Whenever you're ready, feel free to open your PR (as a draft). Make sure that you are working off of a branch on your fork. Our dialogue can continue there.

I also ensured my whole team submitted the CLA, but we understand that may take some time.

Awesome!