Commit ac71675d authored by Filipa Lacerda's avatar Filipa Lacerda

Fixes toggle discussion button not expanding collapsed discussions

Discussions were being toggled by jquery DOM querying them and toggling
visibility but in vue, only the open discussions will be in the DOM
Fix includes:
 - Adds a getter to the store to get the expanded discussions
 - Adds an action to collapse a discussion
 - When the user clicks the button, all data needed is now accessible
   through a getter and we can dispatch an action to toggle the discussion
   within the state, instead of showing/hiding with jQuery
 - Removes hardcoded properties

Resolves #48237
parent cb6bc902
<script> <script>
import _ from 'underscore'; import _ from 'underscore';
import { mapActions, mapGetters } from 'vuex';
import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
import Icon from '~/vue_shared/components/icon.vue'; import Icon from '~/vue_shared/components/icon.vue';
import FileIcon from '~/vue_shared/components/file_icon.vue'; import FileIcon from '~/vue_shared/components/file_icon.vue';
...@@ -54,6 +55,10 @@ export default { ...@@ -54,6 +55,10 @@ export default {
}; };
}, },
computed: { computed: {
...mapGetters('diffs', ['diffHasExpandedDiscussions']),
hasExpandedDiscussions() {
return this.diffHasExpandedDiscussions(this.diffFile);
},
icon() { icon() {
if (this.diffFile.submodule) { if (this.diffFile.submodule) {
return 'archive'; return 'archive';
...@@ -88,9 +93,6 @@ export default { ...@@ -88,9 +93,6 @@ export default {
collapseIcon() { collapseIcon() {
return this.expanded ? 'chevron-down' : 'chevron-right'; return this.expanded ? 'chevron-down' : 'chevron-right';
}, },
isDiscussionsExpanded() {
return this.discussionsExpanded && this.expanded;
},
viewFileButtonText() { viewFileButtonText() {
const truncatedContentSha = _.escape(truncateSha(this.diffFile.contentSha)); const truncatedContentSha = _.escape(truncateSha(this.diffFile.contentSha));
return sprintf( return sprintf(
...@@ -113,6 +115,7 @@ export default { ...@@ -113,6 +115,7 @@ export default {
}, },
}, },
methods: { methods: {
...mapActions('diffs', ['toggleFileDiscussions']),
handleToggle(e, checkTarget) { handleToggle(e, checkTarget) {
if ( if (
!checkTarget || !checkTarget ||
...@@ -125,6 +128,9 @@ export default { ...@@ -125,6 +128,9 @@ export default {
showForkMessage() { showForkMessage() {
this.$emit('showForkMessage'); this.$emit('showForkMessage');
}, },
handleToggleDiscussions(){
this.toggleFileDiscussions(this.diffFile);
},
}, },
}; };
</script> </script>
...@@ -216,9 +222,10 @@ export default { ...@@ -216,9 +222,10 @@ export default {
v-if="diffFile.blob && diffFile.blob.readableText" v-if="diffFile.blob && diffFile.blob.readableText"
> >
<button <button
:class="{ active: isDiscussionsExpanded }" :class="{ active: hasExpandedDiscussions }"
:title="s__('MergeRequests|Toggle comments for this file')" :title="s__('MergeRequests|Toggle comments for this file')"
class="btn js-toggle-diff-comments" @click="handleToggleDiscussions"
class="btn"
type="button" type="button"
> >
<icon name="comment" /> <icon name="comment" />
......
...@@ -82,6 +82,36 @@ export const expandAllFiles = ({ commit }) => { ...@@ -82,6 +82,36 @@ export const expandAllFiles = ({ commit }) => {
commit(types.EXPAND_ALL_FILES); commit(types.EXPAND_ALL_FILES);
}; };
/**
* Toggles the file discussions after user clicked on the toggle discussions button.
*
* Gets the discussions for the provided diff.
*
* If all discussions are expanded, it will collapse all of them
* If all discussions are collapsed, it will expand all of them
* If some discussions are open and others closed, it will expand the closed ones.
*
* @param {Object} diff
*/
export const toggleFileDiscussions = ({ getters, dispatch }, diff) => {
const discussions = getters.getDiffFileDiscussions(diff);
const shouldCloseAll = getters.diffHasAllExpandedDiscussions(diff);
const shouldExpandAll = getters.diffHasAllCollpasedDiscussions(diff);
discussions.forEach(discussion => {
const data = { discussionId: discussion.id };
if (shouldCloseAll) {
dispatch('collapseDiscussion', data, { root: true });
} else if (shouldExpandAll) {
dispatch('expandDiscussion', data, { root: true });
} else if (!shouldCloseAll && !shouldExpandAll && !discussion.expanded) {
dispatch('expandDiscussion', data, { root: true });
}
});
};
export default { export default {
setBaseConfig, setBaseConfig,
fetchDiffFiles, fetchDiffFiles,
...@@ -92,4 +122,5 @@ export default { ...@@ -92,4 +122,5 @@ export default {
loadMoreLines, loadMoreLines,
loadCollapsedDiff, loadCollapsedDiff,
expandAllFiles, expandAllFiles,
toggleFileDiscussions,
}; };
import _ from 'underscore';
import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants';
export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE; export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE;
...@@ -8,5 +9,38 @@ export const areAllFilesCollapsed = state => state.diffFiles.every(file => file. ...@@ -8,5 +9,38 @@ export const areAllFilesCollapsed = state => state.diffFiles.every(file => file.
export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null); export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null);
// prevent babel-plugin-rewire from generating an invalid default during karma tests /**
* Checks if the diff has all discussions expanded
* @param {Object} diff
* @returns {Boolean}
*/
export const diffHasAllExpandedDiscussions = (state, getters) => diff => {
const discussions = getters.getDiffFileDiscussions(diff);
return (discussions.length && discussions.every(discussion => discussion.expanded)) || false;
};
/**
* Checks if the diff has all discussions collpased
* @param {Object} diff
* @returns {Boolean}
*/
export const diffHasAllCollpasedDiscussions = (state, getters) => diff => {
const discussions = getters.getDiffFileDiscussions(diff);
return (discussions.length && discussions.every(discussion => !discussion.expanded)) || false;
};
/**
* Returns an array with the discussions of the given diff
* @param {Object} diff
* @returns {Array}
*/
export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) => diff =>
rootGetters.discussions.filter(
discussion =>
discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash),
) || [];
// prevent babel-plugin-rewire from generating an invalid default during karma∂ tests
export default () => {}; export default () => {};
...@@ -15,6 +15,8 @@ let eTagPoll; ...@@ -15,6 +15,8 @@ let eTagPoll;
export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data); export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data);
export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data);
export const setNotesData = ({ commit }, data) => commit(types.SET_NOTES_DATA, data); export const setNotesData = ({ commit }, data) => commit(types.SET_NOTES_DATA, data);
export const setNoteableData = ({ commit }, data) => commit(types.SET_NOTEABLE_DATA, data); export const setNoteableData = ({ commit }, data) => commit(types.SET_NOTEABLE_DATA, data);
......
...@@ -17,6 +17,10 @@ export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION'; ...@@ -17,6 +17,10 @@ export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION';
export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES'; export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES';
export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE'; export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
// DISCUSSION
export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
// Issue // Issue
export const CLOSE_ISSUE = 'CLOSE_ISSUE'; export const CLOSE_ISSUE = 'CLOSE_ISSUE';
export const REOPEN_ISSUE = 'REOPEN_ISSUE'; export const REOPEN_ISSUE = 'REOPEN_ISSUE';
......
...@@ -58,6 +58,11 @@ export default { ...@@ -58,6 +58,11 @@ export default {
discussion.expanded = true; discussion.expanded = true;
}, },
[types.COLLAPSE_DISCUSSION](state, { discussionId }) {
const discussion = utils.findNoteObjectById(state.discussions, discussionId);
discussion.expanded = false;
},
[types.REMOVE_PLACEHOLDER_NOTES](state) { [types.REMOVE_PLACEHOLDER_NOTES](state) {
const { discussions } = state; const { discussions } = state;
......
---
title: Fixes toggle discussion button not expanding collapsed discussions
merge_request:
author:
type: fixed
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment