Make file templates easy to use and discover
This MR makes file template selectors be displayed for all new files created, and when editing a file for which templates exist. It also adds a "Template type selector" and the ability to Undo applying a template.
Creating a new file:
Editing a file:
A note to FE reviewers:
Previously, the TemplateSelector class was extended for each template type and responsible for managing state for the entire page. This MR moved much of that coordination logic to the FileTemplateMediator and reduced the responsibility of individual template selectors (FileTemplateSelector) to 1) initializing dropdowns and 2) exposing methods for the mediator to manage their UI state.
Using Jquery and plain JS classes to manage the UI state here will become increasingly difficult. IMO the UI is already complex enough to justify refactoring the entire file editor to use Vue components. I think it will be especially necessary for additional features to be added. I opted not to do that now so we could fit this into the coming release, and because most of the logic required for this feature already existed in the code. I opened https://gitlab.com/gitlab-org/gitlab-ce/issues/30192 to address this. cc: @alfredo1
Closes #25332 (closed)
Merge request reports
Activity
added 1 commit
- 978e7fc5 - Convert template selectors to module syntax, refactor to es6.
mentioned in merge request !9792 (merged)
added 468 commits
-
978e7fc5...6619772f - 461 commits from branch
master - fec317fa - Lightly refactor js files related to file template selectors.
- 977e90a6 - Remove unneeded super references.
- cb990178 - Use direct method reference in dlDropdown config.
- 1ce9ee7d - Revert "Use direct method reference in dlDropdown config."
- 41cf6e29 - Re-add missing event reference.
- b34c1295 - Update wrapper ref to $dropdownContainer.
- e3ec96f2 - Stub out FileTemplateMediator.
Toggle commit list-
978e7fc5...6619772f - 461 commits from branch
assigned to @brycepj
added 1 commit
- 6338d95f - Further stub out file_template_mediator and file_template_selector.
added 159 commits
-
6338d95f...4bf4612c - 153 commits from branch
master - d3eaaf05 - Lightly refactor js files related to file template selectors.
- 1feb09cf - Small cleanup. Squash before merge.
- 1ca48027 - Use direct method reference in dlDropdown config.
- a8212f7e - Revert "Use direct method reference in dlDropdown config."
- d37f7677 - Stub out FileTemplateMediator.
- 244d29f2 - Further stub out file_template_mediator and file_template_selector.
Toggle commit list-
6338d95f...4bf4612c - 153 commits from branch
Currently when you create or edit a file (blob), the selection and loading of file templates is handled by the
TemplateSelectorclass. Its 4 subclasses are instantiated (by 4 initializing classes) when a file is being edited or created.There are many problems that arise out of the fact that template selectors are not aware of each other:
- They are all fully instantiated (including each instance of
glDropdown) on pageload, although they may never be displayed. In fact, it is impossible for them to be displayed when an existing file is being edited, but they are instantiated anyway. - They are all listening to keyup events on the filename input and handling each event independently
- They are all responsible for managing page-level events and manipulating the DOM in various ways
- There are quite a few layers of inheritance you need to move between to accomplish simple tasks
Problems with this:
- Lots (4x) of duplicated work on pageload and keypress events
- Poor separation of concerns and encapsulation of data
- Poor adherence to the single responsibility principle
- Unnecessary code complexity
I'm proposing a somewhat simple architectural change: that a
mediatorclass be inserted above the template selectors (more info on this design pattern here), which will be responsible for managing the flow of data to and from template selectors, server endpoints, and the rest of the page. Template selectors will only be responsible for reporting when templates have been selected and exposing methods for the mediator class to work with.This is important because a %9.1 deliverable includes adding a template type selector, adding several conditions for when template selectors are hidden and displayed, and adding a 'Confirm' step, before overwriting existing content with a template. In short, this will mean much more data-layer and DOM-layer state to manage.
It's also worth noting that the goal of the deliverable is for file templates to be used more. If that goal is achieved, it is likely that more types of file templates will be added. The existing architecture will not support scaling.
Before I started, I drew up a couple diagrams to make sure it made sense it my head. Here's a picture:
I don't think Vue is neccessary here, and can explain why. But I'm open to other opinions on that.
Does this make sense? Sorry if something isn't clear. Questions/concerns?
Edited by username-removed-408230- They are all fully instantiated (including each instance of
mentioned in merge request !9888 (closed)
@brycepj thank you so much for explaining. I'm not aware of the internals, but the way you explained it makes sense to me. My concerns are:
- Do you have everything you need to proceed with this refactor? If not, who else should pitch in?
- With this new information, can we still consider #25332 (closed) a Deliverable for %9.1? /cc @mydigitalself
-
@pedroms oh, yeah, I added that note for frontend architecture specialists to review. I'm almost done implementing it actually. Should be no reason we'll need to delay.
@brycepj Nice, thank you for the update!
added 89 commits
-
f0d190e4...a1a04828 - 85 commits from branch
master - f2bdff04 - Lightly refactor js files related to file template selectors.
- d2c666f1 - Use import syntax rather than require.
- a42e240e - Pick changes from !9888 (closed)
- 3cc7418c - Get file template mediator prototype running.
Toggle commit list-
f0d190e4...a1a04828 - 85 commits from branch
added 2 commits
@brycepj so, according to your comment, this MR also closes https://gitlab.com/gitlab-org/gitlab-ce/issues/20303, correct?
-
@pedroms Well, I already closed it, but yes, this MR will encapsulate the functionality there. The
stretchgoal here isn't too big of a stretch.Edited by username-removed-408230 added 1 commit
- bbef6e94 - Get file template preview prototype running.
-
@pedroms @mydigitalself You can check out this branch and try out the feature. I have a bunch of code cleanup to do, but everything should work as expected. Let me know if this helps clarify questions about https://gitlab.com/gitlab-org/gitlab-ce/issues/25332 or if you have feedback of any kind.
added 1 commit
- 526e3594 - Fix spacing between file-editor and page-title.
added 1 commit
- 91147ba1 - Replace template preview with template undo.
@brycepj thanks a lot! I was able to test it and most of it works and looks as expected, awesome
🙂 Here are some things that I found were lacking:- Order the template types alphabetically (.gitignore, .gitlab-ci.yml, etc.)
- Add a bit of spacing between the dropdowns and the editor so it's the same between the dropdowns and the navigation bar (view guideline or view proposed design):
- Move the dropdowns a bit up so they are centered with the “Template” and “New file” text (view guideline)
- If I'm creating a file, apply a template, change something in the file name or content, and then apply another template, I would expect to have the “Undo” option visible. Pressing “Undo” would allow me to revert to the previous template.
There are still somethings missing from the scope of https://gitlab.com/gitlab-org/gitlab-ce/issues/20303, which I'm not sure if you will include in this MR. I'll list them here for your reference anyway:
- In the “New file” screen add two tabs: “Write” and “Preview”
- When the user is creating any Markup files from these extensions and switches to the “Preview” tab, it should render the content as HTML
- When the user is creating any non-markup files, they can see syntax highlighted content of that file by switching to the “Preview” tab
- In both the “New file” and “Edit file” screens: restyle the tabs according to the designs.
Please let me know if any of this doesn't make sense or isn't feasible.
-
@pedroms all great feedback! thanks
♥️ As for the first set of suggestions, I agree on all counts. I'll work on those things now.
As for the second set, I'm thinking we should do the #20303 (closed) work separately -- I think I said somewhere earlier that this MR encompassed that issue, but I really only had in mind Confirming/Canceling templates before they're applied. There's a fair amount of work to start rendering markup and syntax highlighted content. I know that work was started in !7938 (closed), but it's not quite finished, and will need some refactoring to make it fit with the upgrades we're doing here.
Edited by username-removed-408230 marked the task Add a bit of spacing between the dropdowns and the editor so it's the same between the dropdowns and the navigation bar (view guideline or view proposed design): as completed
marked the task Move the dropdowns a bit up so they are centered with the “Template” and “New file” text (view guideline) as completed
added 2 commits
-
@pedroms @mydigitalself Okay, I've made all the fixes/changes I saw in your feedback. Pull in my recent changes, play around with it some more, and let me know what you think. If this all looks good, I'll focus on cleaning up the code and writing tests from here on out.
Edit: I just realized that styling needs some tweaks for smaller screens. Working on it now.
Edit: Responsive styling done: 9f25bab9
Edited by username-removed-408230 added 1 commit
- 9f25bab9 - Improve template selector styles for smaller viewports.
@brycepj seems a lot more intuitive now, nice one!
Found one bug, if you start a new file, then immediately choose a template and Undo, it doesn't Undo, but the undo button vanishes.
-
@brycepj Ok, so let's do https://gitlab.com/gitlab-org/gitlab-ce/issues/20303 separately. @nmrony was leading the MR for that issue (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7938), so can you please follow up with him since it won't be included in this MR?
🙏 😉 Some notes from my latest tests:
- When undoing, can we also reset the dropdowns?
- I found it a bit odd when undoing in the “Preview” tab, because the file name changes but the preview stays the same. Can we refresh the preview when undoing? If not, can we switch to the “Write” tab?
Two “stylistic” things missing:
- Little nitpick: the dropdowns have the same space above and below, but now the text “New|Edit file” and “Template” are a bit up. See current and aligned
- Style the “Write” and ”Preview changes” tabs according to the proposed design so that the new template selectors don't create an unbalanced layout
added 1 commit
- 33d082bd - Fix and add specs for undo applying templates.
added 452 commits
-
33d082bd...7c2dd5d4 - 449 commits from branch
master - 5524bc63 - Clean application of file templates on master.
- ba13193d - Add undo template spec file.
- f7156ac3 - Fix TemplateSelector reference, include old template_selector.
Toggle commit list-
33d082bd...7c2dd5d4 - 449 commits from branch
added 1 commit
- fcebcbf1 - Clean application of file templates on master.
-
When undoing, can we also reset the dropdowns?
Yes, done now.
I found it a bit odd when undoing in the “Preview” tab, because the file name changes but the preview stays the same. Can we refresh the preview when undoing? If not, can we switch to the “Write” tab?
Interesting -- I need to look at what it would require to trigger a re-rendering of the new content. But I agree, we should refine this.
Little nitpick: the dropdowns have the same space above and below, but now the text “New|Edit file” and “Template” are a bit up. See current and aligned
I appreciate your nitpick-ery. Good eye
😉 Fixed here: 223f2c3dStyle the “Write” and ”Preview changes” tabs according to the proposed design so that the new template selectors don't create an unbalanced layout
Thanks for pointing this out. Fixed: 82c75a4d
Edited by username-removed-408230 -
@pedroms I'm looking at the issue you raised about the Preview tab, and it looks we're also not even applying the template properly if the user applies a new template while in Preview tab.
I'm wondering if it would be reasonable to hide all of the template selectors when the user is in preview mode? Allowing templates to be applied and undone while previewing adds a fair amount of complexity to the UI state, without (as far as I can tell) adding much value. That sort of functionality seems to fit a little better with upcoming changes related to https://gitlab.com/gitlab-org/gitlab-ce/issues/20303. WDYT?
I could also see hiding template selectors during preview as a temporary thing. I'm actually getting ready to open an issue to refactor the code for the file editor to use Vue, which would make it much easier to manage complex UI state. As it is, we're working in legacy code that makes such things more difficult and time consuming. I'm just trying to mindful of the upcoming feature freeze (Apr 7th), and wanting to ensure this makes it in.
cc: @mydigitalself
Edited by username-removed-408230 @brycepj Cool, thank you for fixing all of those things. I haven't had the time to review it again, but will try to do so tomorrow.
Regarding the Preview tab: sure, let's hide the template selectors and label when in the Preview tab. We can refine it later if we need to. Thanks!
- Edited by username-removed-408230
mentioned in issue #30192 (moved)
added 1 commit
- 1514e25e - Clean up FileTemplateSelector and FileTemplateMediator.
added 2 commits
@brycepj Yes, it's better
🙂 I don't want to hold on this MR because of such small things, but… for perfect alignment, the “New|Edit file” title should go down by ~1px and the Template label down by ~2px. Basically, the text baseline should be the same between title, label and selects. But again, you feel like this is too much nitpick-ery, leave it as it is. Thanks, it's working/looking great👍 added 1 commit
- 57e6f9f4 - Improve alignment on page title for New/Edit blob.
- Edited by username-removed-408230
added 154 commits
-
7782b89e...c189bf11 - 137 commits from branch
master - acd0b3f1 - Clean application of file templates on master.
- 35e7a2a2 - Style nav-links for better contrast.
- 2810ffbb - Hide template selector menu on Preview state.
- e14138a9 - Fix vertical alignment of title & template selector menu.
- f7116264 - Prevent collapse of page title when template selectors not shown.
- 3e2ab528 - Add show/hide template selector menu based on matching.
- 8169b857 - Clean up FileTemplateSelector and FileTemplateMediator.
- d6d67945 - Revert removing default for skipFocus.
- 2154013e - Fix breaking changes.
- 0ae96e5b - Cache blob before dropdown updates toggleText.
- d967f01b - Fix toggle text restore from cache.
- 8514d24a - Fix template type dropdown spec display mixup.
- ac7b808b - Fix undo button causing bump in menu height.
- 3f9671df - Shush eslint, rubocop, scss_lint.
- 5c3a71ee - Improve alignment on page title for New/Edit blob.
- fac4587e - Clean up editor scss.
- e62d6810 - Improve template mediator naming.
Toggle commit list-
7782b89e...c189bf11 - 137 commits from branch
@brycepj Hum… it looks more misaligned now. My HEAD is at https://gitlab.com/gitlab-org/gitlab-ce/commit/e62d68101b21f1b570967125057e6d7cde625fd8
-
@pedroms Okay, I think I fixed it (https://gitlab.com/gitlab-org/gitlab-ce/commit/3b055cc781858a6a90362f3707299dc2d84545b4), but I also improved the CSS so that moving the individual pieces up and down just requires changing the
margin-top/marginproperty for a given element. If you still see that we need to adjust a pixel here or there, are you comfortable enough with CSS to change the code directly? I just don't quite have the eye for this level of detail🙈 Edited by username-removed-408230 @brycepj thanks, I've changed a few things and everything is aligned now. However, upon testing the responsive-ness, I've noticed that it breaks
😮 What can we do here?
-
@pedroms I like your style changes... thanks for doing that! The responsiveness issues should be fixed now.
Edited by username-removed-408230 -
@alfredo1 Just FYI, CI is re-running bc of some small style changes @pedroms and I made, but the most recent build is passing.
Edited by username-removed-408230 @brycepj amazing job
🌟 ! Code looks great so far. I left a few comments for you.assigned to @brycepj
@brycepj responsive-wise, it looks great!
👍 Great job, thanksadded 1 commit
- a0013d22 - Remove unused logic for autosize and setEditorContent.
added 1 commit
- f667470f - Use target href instead of hash to match preview tab.
-
@alfredo1 all discussions now resolved. Can you review my changes? They're pretty small.
Edited by username-removed-408230 This is failing because of https://gitlab.com/gitlab-org/gitlab-ce/issues/30236. Hopefully the fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10337 gets merged soon.
Edited by username-removed-408881@brycepj Code looks pretty solid to me, excellent work! Let's wait until !10337 (merged) gets merged so you can rebase with master to make this MR green.
changed milestone to %9.1
added frontend label
-
Code looks pretty solid to me, excellent work! Let's wait until !10337 (merged) gets merged so you can rebase with master to make this MR green.
@alfredo1 thanks for the tip. I'll keep an eye out for that and ping you when it's been rebased.
assigned to @brycepj
added 1 commit
- 9b51eda6 - Remove no-new annotation from file_template_mediator.js.
added 161 commits
-
9b51eda6...731d574d - 119 commits from branch
master - af316e81 - Clean application of file templates on master.
- fd99d3fb - Style nav-links for better contrast.
- 91f189b7 - Hide template selector menu on Preview state.
- 57f2952e - Fix vertical alignment of title & template selector menu.
- e4a61f91 - Prevent collapse of page title when template selectors not shown.
- cc49828b - Add show/hide template selector menu based on matching.
- 5ca72449 - Clean up FileTemplateSelector and FileTemplateMediator.
- c5cea71f - Revert removing default for skipFocus.
- 3e34dbe4 - Fix breaking changes.
- a53b45ff - Cache blob before dropdown updates toggleText.
- 2bd72c7b - Fix toggle text restore from cache.
- 545eec72 - Fix template type dropdown spec display mixup.
- 07d82907 - Fix undo button causing bump in menu height.
- db2a312b - Shush eslint, rubocop, scss_lint.
- f7233aa2 - Improve alignment on page title for New/Edit blob.
- d88b0af3 - Clean up editor scss.
- 7b30477a - Improve template mediator naming.
- 6f254248 - Vertically align page title components.
- 49f51d75 - Fix undo menu alignment styling.
- 9cec711d - Rename listenForPreviewState to listenForPreviewMode.
- 576d387c - Improve naming and cleanup template_type_dropdown_spec.
- 3e7a3689 - Cleanup and improve naming for undo_template_spec.
- aaa63459 - Remove scss whitespace.
- 271b9b79 - Update failing spec refs from 'New FIle' to 'New file'.
- 99293416 - Update refs to "Choose a" to "Apply a".
- f0b693e0 - Fix lingering "New File" ref.
- c16c9652 - Fix responsiveness style issues with undo state.
- 697bc5fc - Align page title and template selectors.
- d4c2409a - Remove inline styles.
- 4327bc54 - Prevent default navigation on template type selection.
- e2893fc5 - Prevent default when selectTemplate triggered by click.
- 3785b89c - Fix navlinks selector ref.
- 0573b3cf - Remove whitespace.
- 844f5952 - Add spec for hiding template selectors when user switches to preview state.
- eaf9b014 - Remove stray focus annotation.
- c5be8026 - Move DOM elements out of object.
- 1641c0a6 - Rename cachedTitle to cachedFilename.
- 7bd74a8b - Condense template selector selection classes.
- 8da79c3f - Remove unused logic for autosize and setEditorContent.
- 0e8da840 - Fix bad refs from undoMenu transform.
- 2ce0aece - Use target href instead of hash to match preview tab.
- 3207997f - Remove no-new annotation from file_template_mediator.js.
Toggle commit list-
9b51eda6...731d574d - 119 commits from branch
-
@alfredo1 Just rebased against the fix on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10337 -- is this ready to be MWPS?
Edited by username-removed-408230 @brycepj Can you add a CHANGELOG file?
enabled an automatic merge when the pipeline for 8d34a72e succeeds
Thanks @brycepj
added 90 commits
-
8d34a72e...2fceb437 - 47 commits from branch
master - c464769e - Clean application of file templates on master.
- 2634123b - Style nav-links for better contrast.
- 41861541 - Hide template selector menu on Preview state.
- 6f596b88 - Fix vertical alignment of title & template selector menu.
- 6f029b15 - Prevent collapse of page title when template selectors not shown.
- 1fa833f1 - Add show/hide template selector menu based on matching.
- 3ef4295c - Clean up FileTemplateSelector and FileTemplateMediator.
- d81d4216 - Revert removing default for skipFocus.
- 1cf488d5 - Fix breaking changes.
- 25e7eddc - Cache blob before dropdown updates toggleText.
- bbc44048 - Fix toggle text restore from cache.
- 50c7860a - Fix template type dropdown spec display mixup.
- 648f5c4c - Fix undo button causing bump in menu height.
- f136240e - Shush eslint, rubocop, scss_lint.
- bcc6a60f - Improve alignment on page title for New/Edit blob.
- 4111291a - Clean up editor scss.
- 23fdf173 - Improve template mediator naming.
- f0509d42 - Vertically align page title components.
- e00d6b1b - Fix undo menu alignment styling.
- 2a1c6752 - Rename listenForPreviewState to listenForPreviewMode.
- c9edf520 - Improve naming and cleanup template_type_dropdown_spec.
- cc9c51c4 - Cleanup and improve naming for undo_template_spec.
- e82f6cd7 - Remove scss whitespace.
- 95dd0b8d - Update failing spec refs from 'New FIle' to 'New file'.
- 70552d2b - Update refs to "Choose a" to "Apply a".
- 6baa9ce8 - Fix lingering "New File" ref.
- e949268c - Fix responsiveness style issues with undo state.
- 7f6e6527 - Align page title and template selectors.
- 8099357d - Prevent default navigation on template type selection.
- 4f620423 - Prevent default when selectTemplate triggered by click.
- aad98a6b - Fix navlinks selector ref.
- ce75819d - Remove whitespace.
- be1a6478 - Add spec for hiding template selectors when user switches to preview state.
- 2a498a08 - Remove stray focus annotation.
- 0ceff293 - Move DOM elements out of object.
- 433765e1 - Rename cachedTitle to cachedFilename.
- a3ad34e7 - Condense template selector selection classes.
- f1d8eb8f - Remove unused logic for autosize and setEditorContent.
- 2668e0ad - Fix bad refs from undoMenu transform.
- b671d69e - Use target href instead of hash to match preview tab.
- c695f03b - Make file templates easy to use and discover
- 028256bf - Remove no-new annotation from file_template_mediator.js.
- 773876ab - Add MR number to changelog.
Toggle commit list-
8d34a72e...2fceb437 - 47 commits from branch
-
@alfredo1 Just rebased to fix a failure from master. Can you approve and set to MWPS again?
🙏 Edited by username-removed-408230 enabled an automatic merge when the pipeline for 773876ab succeeds
-
thanks @alfredo1!
Looks like tests passed -- once post-test reports (1 & 2) are generated, this will be merged.
🎊 Edited by username-removed-408230 mentioned in commit 97c49b84
@brycepj finally
🎉 mentioned in issue #29418 (closed)
mentioned in issue #29462 (closed)
