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

add a tab with default extensions to extension manager #13136

Merged
merged 8 commits into from
Apr 13, 2017

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Mar 2, 2017

Adds a new tab that allows disabling/enabling default extensions (this functionality is already available, so this PR is mostly about displaying the list of default extensions).

Maybe we could use a different icon but I'm not capable of producing one and this can be replaced in a separate PR after this is merged.

Tagging @swmitra @redmunds to review as per discussion in #13108 but others feel free to chime in.

image

@ficristo
Copy link
Collaborator

ficristo commented Mar 2, 2017

I like how things come back #11618
I gave a try and seems I cannot disable the default Dark theme or JSLint.

@zaggino
Copy link
Contributor Author

zaggino commented Mar 2, 2017

Interesting, did you try it on Mac or Windows @ficristo ?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 2, 2017

The thing it came back again probably means we should finish it this time 😄

@petetnt
Copy link
Collaborator

petetnt commented Mar 2, 2017

Nice @ficristo! I remembered that PR but couldn't find it myself.

👍 for the idea from me. Adding descriptions for the default extensions that don't have them would be nice.

@redmunds
Copy link
Contributor

redmunds commented Mar 2, 2017

I think disabling the Default Theme should not be allowed. Isn't that what gets used when you Restart Without Extensions ?

Also, JSLint extension is enabled by default. Maybe code needs to verify existence of linting module before trying to use it?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 2, 2017

Disabling themes (as I agree it makes little sense) has been forbidden in 2d197fb

@zaggino zaggino force-pushed the zaggino/disable-default-extensions branch from 8a3eab0 to 3d58d8e Compare March 2, 2017 23:48
@zaggino
Copy link
Contributor Author

zaggino commented Mar 2, 2017

Ok guys, did a few changes, please have a look. I've tested disabling/enabling JSLint and it works fine.
I've also tested disabling all the default extensions (except themes which now can't be disabled) and Brackets did startup just fine. I've added a warning to the console logs about the default extension being disabled as it might help with bug reports.

@petetnt adding descriptions would definitely be nice but I believe that can be handled as a starter issue.

@redmunds
Copy link
Contributor

redmunds commented Mar 3, 2017

@zaggino Built code, did minimal testing, and seems to work as advertised. Made a pass through code -- looks good. Awesome!

@ficristo
Copy link
Collaborator

ficristo commented Mar 3, 2017

I tryed again and JSLint is disabled by default and I cannot enabled. Is it normal or it is maybe related to my system?
In the preferences I have: "extensions.default.disabled": []

@zaggino
Copy link
Contributor Author

zaggino commented Mar 3, 2017

@ficristo if you have tried previous version, look at JSLint's directory if there's not a .disabled filename present and delete it or run git clean -fdx && npm install ... I had the same issue and this fixed it.

@ficristo
Copy link
Collaborator

ficristo commented Mar 4, 2017

Running git clean fixed it.
I wanted to disable JSLint only for Brackets so I added the preference to disable it in .brackets.json.
But JSLint is still available. Is this configuration not supported?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 4, 2017

I don't think this could work per project @ficristo if that's what you're trying to do. The place where the extensions are being loaded does not consider which project you're in and neither does this get executed when you switch projects. While it wouldn't be hard to enable an extension when you switch to other project, it would be very difficult to disable one when you switch to a project which has it disabled in .brackets.json because disabling an extension basically means reloading whole editor. Now I know VSCode does that, maybe even Atom (reload whole editor when switching projects) but we currently don't and that'd be potentially huge change how Brackets work.

@zaggino
Copy link
Contributor Author

zaggino commented Mar 7, 2017

Any more comments @adobe/brackets-committers ?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 12, 2017

@swmitra can anyone in your team review this please?

@zaggino zaggino added this to the Release 1.10 milestone Mar 23, 2017
@justinrusso
Copy link
Contributor

justinrusso commented Apr 3, 2017

With PR #13245 merged, the @extension-manager-min-width needs adjusting. Looks like 750px is good.

Note: This is located in src/styles/brackets_variables.less

@swmitra
Copy link
Collaborator

swmitra commented Apr 5, 2017

Very nice @zaggino 👍.
Tested this branch just now and everything seems to be working fine. I just have one small comment (But that would not block me to merge this PR now :smile), If we can cook some description for the default extensions to provide some meaningful info to the user based on which they can decide whether to disable any default extension!

Copy link
Collaborator

@swmitra swmitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@swmitra
Copy link
Collaborator

swmitra commented Apr 5, 2017

@zaggino Can you please increase the min-width of the EM Dialog to ~760px?

@zaggino
Copy link
Contributor Author

zaggino commented Apr 13, 2017

@swmitra where's this min-width set?

I can see that EM has the width hard set to 760px here: https://github.com/adobe/brackets/blob/master/src/styles/brackets_patterns_override.less#L1026-L1029

@zaggino
Copy link
Contributor Author

zaggino commented Apr 13, 2017

I'll add descriptions to the default extensions in another PR, to prevent ongoing discussions what it should be :)

@zaggino
Copy link
Contributor Author

zaggino commented Apr 13, 2017

ah, found it @swmitra @justinrusso please check if d6cf7c0 is correct

@swmitra
Copy link
Collaborator

swmitra commented Apr 13, 2017

@zaggino Yes that's the place. I was about to reply but you are fast 😄 I have tested this feature and works as expected. Merging ...

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

6 participants