Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

CSSInlineEditor class should be renamed and moved to separate file #768

Closed
gruehle opened this issue Apr 28, 2012 · 4 comments
Closed

CSSInlineEditor class should be renamed and moved to separate file #768

gruehle opened this issue Apr 28, 2012 · 4 comments

Comments

@gruehle
Copy link
Member

gruehle commented Apr 28, 2012

The CSSInlineEditor class is really just a generic multi-file inline text editor that can handle other types of content. For example, it can be used, unmodified, to display JavaScript function entries instead.

The class should be renamed to something generic (along the lines of MultiFileInlineTextEditor), and moved to a separate file. The CSSInlineEditor module should only contain the htmlToCSSProvider code.

@peterflynn
Copy link
Member

Another option is to push down all the generic stuff into the InlineTextEditor base class (and maybe rename it). It already encapsulates the notion of having multiple Editors, so it doesn't feel like a stretch to also include the list UI (driven by TextRanges) and editor-switching logic. As it stands, InlineTextEditor feels like an awkward link in the inheritance chain, since it provides some of the tools for a multi-Editor UI but not all of them.

This would be basically equivalent to doing what Glenn said above, and then merging that new MultiFileInlineTextEditor with the existing InlineTextEditor (since it also implies multiple editors).

@gruehle
Copy link
Member Author

gruehle commented Apr 30, 2012

I agree that InlineTextEditor is awkward at the moment. However, there are some key questions that need to be answered about it:

  1. Will we support multiple text ranges visible at the same time? (this is in the Northstar, but hasn't been committed for Brackets yet)
  2. Do we want a base class that only shows a single text range (ie no list on the right)?
  3. Do we want a base class that has a file header, but doesn't have a text editor? (ie for showing other file types like image)

I'm hesitant to merge CSSInlineEditor into InlineTextEditor without knowing the answers to these questions. Simply moving CSSInlineEditor to a separate file (and giving it a new name) has immediate benefit and low cost, and does not rule out further refactoring/merging with InlineTextEditor.

@peterflynn
Copy link
Member

Would it be reasonable to hoist the multi-editor-related pieces of InlineTextEditor up into that new class then? The main thing that bugs me about InlineTextEditor is that its name implies it hosts a single text Editor, but its API is designed around having multiple Editors. (Mainly I'm thinking of _syncGutterWidths(), sizeInlineWidgetToContents(), and the other code that references the editors array).

Or if that feels like too much churn at once we could always drop a TODO in there and file a spinoff bug for that phase of the cleanup...

@peterflynn
Copy link
Member

The current refactoring is a definite improvement, so we can close this. I still think we should clean up / clarify / maybe remove InlineTextEditor at some point, but it can wait.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants