New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#6307 fix page title not changed (stable12) #6987

Merged
merged 2 commits into from Oct 30, 2017

Conversation

Projects
None yet
3 participants
@burned42
Contributor

burned42 commented Oct 27, 2017

Fixes #6307

Backport of #6869

With this changes the page title gets set when switching to file list and tag list.

@danxuliu here is the pull request to bring the bugfix also to stable12.

burned42 added some commits Oct 18, 2017

bugfix: set/change page title when switching to filelist
Signed-off-by: Bernd Stellwag <burned@zerties.org>
bugfix: set/change page title when switching to file tags
Signed-off-by: Bernd Stellwag <burned@zerties.org>
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 27, 2017

Codecov Report

Merging #6987 into stable12 will increase coverage by <.01%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             stable12    #6987      +/-   ##
==============================================
+ Coverage       53.75%   53.75%   +<.01%     
  Complexity      22579    22579              
==============================================
  Files            1384     1384              
  Lines           86623    86624       +1     
  Branches         1329     1329              
==============================================
+ Hits            46568    46569       +1     
  Misses          40055    40055
Impacted Files Coverage Δ Complexity Δ
apps/systemtags/js/systemtagsfilelist.js 73.27% <100%> (+0.23%) 0 <0> (ø) ⬇️

codecov bot commented Oct 27, 2017

Codecov Report

Merging #6987 into stable12 will increase coverage by <.01%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             stable12    #6987      +/-   ##
==============================================
+ Coverage       53.75%   53.75%   +<.01%     
  Complexity      22579    22579              
==============================================
  Files            1384     1384              
  Lines           86623    86624       +1     
  Branches         1329     1329              
==============================================
+ Hits            46568    46569       +1     
  Misses          40055    40055
Impacted Files Coverage Δ Complexity Δ
apps/systemtags/js/systemtagsfilelist.js 73.27% <100%> (+0.23%) 0 <0> (ø) ⬇️
@danxuliu

danxuliu approved these changes Oct 28, 2017 edited

Tested and works 👍

While reviewing this again I have noticed that the page title for the All files section was already working as expected without this patch (both in master and in stable12) due to the change introduced in #6683/#6689. However, as said in #6869 (comment), even if the call is redundant it makes the code more consistent with other file lists, so I am fine with merging this now and improving the overall file list code later.

@danxuliu

This comment has been minimized.

Show comment
Hide comment
@danxuliu

danxuliu Oct 28, 2017

Member

Thanks again, @burned42 :-D

By the way, as you may know, "Fixes #issueNumber" makes GitHub automatically close the referenced issue when the pull request is merged. In #6869 I changed your original comment from "Fixes #6307" to "Fixes (on master) #6307" to break the parsing and ensure that merging that pull request would not automatically close #6307, because it was filled against Nextcloud 12 but the pull request was for master. However, this pull request is for stable12, so I have changed "Fixes (on stable12) #6307" to "Fixes #6307" to ensure that it automatically closes the issue ;-)

I have also added "Backport of #6869" to ease tracking of the pull requests between branches :-)

Member

danxuliu commented Oct 28, 2017

Thanks again, @burned42 :-D

By the way, as you may know, "Fixes #issueNumber" makes GitHub automatically close the referenced issue when the pull request is merged. In #6869 I changed your original comment from "Fixes #6307" to "Fixes (on master) #6307" to break the parsing and ensure that merging that pull request would not automatically close #6307, because it was filled against Nextcloud 12 but the pull request was for master. However, this pull request is for stable12, so I have changed "Fixes (on stable12) #6307" to "Fixes #6307" to ensure that it automatically closes the issue ;-)

I have also added "Backport of #6869" to ease tracking of the pull requests between branches :-)

@danxuliu

This comment has been minimized.

Show comment
Hide comment
@danxuliu

danxuliu Oct 28, 2017

Member

Please review @nextcloud/javascript :-)

Member

danxuliu commented Oct 28, 2017

Please review @nextcloud/javascript :-)

@burned42

This comment has been minimized.

Show comment
Hide comment
@burned42

burned42 Oct 28, 2017

Contributor

@danxuliu Apparently I didn't know that GitHub will automatically close the issue based on some specific text in the pull request. But I did notice that you changed the text in the other pull request and that is why I wrote it like that here. But thanks for clarifying and making the according changes :)

Contributor

burned42 commented Oct 28, 2017

@danxuliu Apparently I didn't know that GitHub will automatically close the issue based on some specific text in the pull request. But I did notice that you changed the text in the other pull request and that is why I wrote it like that here. But thanks for clarifying and making the according changes :)

@MorrisJobke

Tested and works 👍

@MorrisJobke MorrisJobke merged commit 0ce7187 into nextcloud:stable12 Oct 30, 2017

3 checks passed

codecov/patch 100% of diff hit (target 53.75%)
Details
codecov/project 53.75% (+<.01%) compared to 85f612c
Details
continuous-integration/drone/pr the build was successful
Details

@MorrisJobke MorrisJobke added this to the Nextcloud 12.0.4 milestone Oct 30, 2017

@MorrisJobke MorrisJobke referenced this pull request Nov 20, 2017

Merged

12.0.4 RC #7225

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment