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

Simple Themes usability tweaks #9069

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

peterflynn
Copy link
Member

Main improvements:

  • Message in Themes dialog: "To get more themes, use Extension Manager." (not clickable at this point -- see discussion in Themes usability improvements #8946)
  • Message in header of Themes tab in Extension Manager: "To switch between installed themes, use View > Themes..."
  • TODO @MiguelCastillo: Installing a theme automatically sets it as the current theme

I threw in two smaller tweaks as well:

  • Link s in Themes dialog to their corresponding controls (they already had a hand cursor thanks to Bootstrap, but clicking them did nothing)
  • Eliminate big blank space at bottom of Themes dialog

Not clickable yet, since the workflows get a little tricky (does it appear
over top of Themes dialog? does dropdown update when you come back? what if
Extension Manager triggers a restart? etc.)
* Show message in Extension Manager about how to switch themes
* Link <label>s in Themes dialog to their corresponding controls
* Eliminate big blank space at bottom of Themes dialog
…s-help

* origin/master: (56 commits)
  remove selection arrow
  fix fullpath check when deleting file
  Fix: toggling Extension Manager long/short description would always show the string for the installed version of an extension (if installed), even when looking at the Available tab. Use info from the current view-model rather than making absolute references to ExtensionManager's model.
  Fix CSSUtils to work better with vendor-prefixed properties
  Added unit tests
  Updated by ALF automation.
  Added unit test
  Use unary operator to sort out invalid numbers
  Use a more strict RegExp
  Updated by ALF automation.
  - Move UI code out of ExtensionManagerView into ExtensionManager - Properly escape extension description after toggling truncated part
  Update CodeMirror to the latest.
  fix 2 issues after merge
  Updated by ALF automation.
  Fix QuickView popover being in the wrong pane
  remove extra cruft
  update getting started screen for ja
  Update strings.js
  target ja/getting started folder
  update getting started for FR, add JA
  ...

Conflicts:
	src/styles/brackets.less
@MiguelCastillo
Copy link
Contributor

@peterflynn I am working through this one right now. There's not a clear path to victory, so it's taking a bit longer than anticipated. Should be done this afternoon though.

@MiguelCastillo
Copy link
Contributor

@peterflynn Code is ready for you to take a look. I refactored a few things... It wasn't as straight forward as I thought it was going to be.

@MiguelCastillo
Copy link
Contributor

@peterflynn I have made the settings in the dialog take effect as they are changed... They are reverted when canceling out of the dialog.

Miguel Castillo added 3 commits September 16, 2014 16:00
@MiguelCastillo
Copy link
Contributor

@peterflynn I got carried away and refactored a few more things in themes... I didn't realize there were that many lose ends or things that just needed TLC. For sure let me know if you got questions.

cc @dangoor

Miguel Castillo added 2 commits September 16, 2014 21:39
…only want to set the current theme when a new one is installed
@MiguelCastillo
Copy link
Contributor

@peterflynn Do we want to move the review only tag?

@MiguelCastillo
Copy link
Contributor

@peterflynn @dangoor Is there anything I need to do to move this one forward?

@peterflynn
Copy link
Member Author

@MiguelCastillo Sorry about the delay! Kevin and I are both away at a conference until Thursday, but hopefully after that we'll be able to take a look shortly after that.

@peterflynn peterflynn changed the title [REVIEW ONLY] Simple Themes usability tweaks Simple Themes usability tweaks Oct 7, 2014
Miguel Castillo added 3 commits October 16, 2014 12:53
conflicts in theme manager and in brackets.less
… was causing issue where I couldn't remove ThemeManager from ExtensionManager
…to a property of the installInfo rather then part of the updateStatus message
@MiguelCastillo
Copy link
Contributor

@peterflynn @dangoor I did some refactoring in Themes to decouple it from the ExtensionManager module. I had coupled it because I didn't know better. I also resolved a circular reference issue with ThemeSettings and ViewCommandHandlers... It was causing issue when I tried to remove ThemeManager from ExtensionManager. There were other refactorings to cleanup the themes code.

I am going to add a unit test for verify that installing a theme will cause that theme to become the current theme.

Here is the flow when installing a theme occurs:
Package calls ExtensionLoader.loadExtension
ExtensionLoader.loadExtension will trigger a "load"/"loadFailed"
ExtensionManager will handle the event in _handleExtensionLoad
ExtensionManager will add the new operationType "install"/"update" in the installInfo.
ExtensionManager will trigger a "statusUpdate" event
ThemeManager will get the "statusUpdate" event and it will determine if a theme is being installed by checking the new "operationType"

@marcelgerber
Copy link
Contributor

@MiguelCastillo You've got some merge conflicts in ExtensionManager.js to resolve.

@MiguelCastillo
Copy link
Contributor

@peterflynn @dangoor This PR has some good code cleanup I have done for themes code... I make the code more maintainable and decouples themes code from the extension manager code. Can we please get this one a bump in priority?

@marcelgerber
Copy link
Contributor

@MiguelCastillo I think this will get it's way into master in the Pull Request burndown.
But it currently contains merge conflicts.

@le717
Copy link
Contributor

le717 commented Nov 21, 2014

@MiguelCastillo You've got a rouge CodeMirror submodule change in here.

@MiguelCastillo
Copy link
Contributor

@dangoor @peterflynn any interest in reviving this?

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

Successfully merging this pull request may close these issues.

None yet

4 participants