Skip to content
Snippets Groups Projects

Fix broken "Show whitespace changes" button on MR "Changes" tab

Merged Jacques Erasmus requested to merge 52122-fix-broken-whitespace-button into master
All threads resolved!

This MR fixes the issue where the Show/Hide whitespace changes button was getting stuck in the Show whitespace changes state once it has been toggled.

The problem was that the w=0 parameter in the URL was getting handled as if it was an integer flag. Since url params are always strings the flag will always result in true regardless of the value.

This MR fixes the issue by converting the w flag to an integer when it is read out.

Tests for the component has also been added.

What are the relevant issue numbers?

Closes #52122 (closed)

Does this MR meet the acceptance criteria?

Edited by Jacques Erasmus

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Lukas Eipert
  • @jerasmus Thank you! Looks great, two comments. Thank you for adding the tests!

  • assigned to @jerasmus

  • Jacques Erasmus added 59 commits

    added 59 commits

    Compare with previous version

  • added 1 commit

    • 2bde04d2 - Move isWhitespaceVisible property into a method

    Compare with previous version

  • added 1 commit

    • 4c48d773 - Add unit tests for comparableDiffs and isWhitespaceVisible

    Compare with previous version

  • added 1 commit

    • 35c51479 - Refactor isWhitespaceVisible method

    Compare with previous version

  • @leipert thanks for the review! I've addressed your comments, please have another look :thumbsup:

  • assigned to @leipert

  • Jacques Erasmus added 10 commits

    added 10 commits

    Compare with previous version

  • @jerasmus LGTM! Moving the thing to a method rather than a computed property seems sensible. I don't know what maintainers think though.

    You should probably run prettier, squash all the commits and set a proper commit message for this smallish change in order to satisfy Danger. In case you need help with that, ping me.

  • assigned to @jerasmus

  • added 1 commit

    • 4873101c - Ran prettier on the changed files

    Compare with previous version

  • Jacques Erasmus added 24 commits

    added 24 commits

    • 4873101c...9791f82b - 9 commits from branch master
    • 64a433b0 - Add changelog entry for broken "Show whitespace changes" button
    • f046a420 - Fix `isWhitespaceVisible` method
    • e4a1f0f5 - Cleanup `isWhitespaceVisible` method
    • 32e188e7 - Cleanup `isWhitespaceVisible` method
    • 8afc1c13 - Add merge requests diffs mock data
    • 89c07b3b - Add unit tests for `compare_version` component
    • 73d1554a - Update unit tests for `compare_version` component
    • 6b9245e3 - Cleanup unit tests for `compare_version` component
    • 5ef8e530 - Remove unused import
    • d204af5c - Change qa class name
    • eba9e72d - Move isWhitespaceVisible property into a method
    • 8a276365 - Add unit tests for comparableDiffs and isWhitespaceVisible
    • 7bb0f73b - Refactor isWhitespaceVisible method
    • 0019a376 - Ran prettier on the changed files
    • dc47bcbf - Merge branch '52122-fix-broken-whitespace-button' of…

    Compare with previous version

  • Jacques Erasmus added 58 commits

    added 58 commits

    Compare with previous version

  • Jacques Erasmus changed milestone to %11.5

    changed milestone to %11.5

  • Jacques Erasmus added 2 commits

    added 2 commits

    • 9ab10a25 - Update links to external sites
    • 786eaa4b - Fix Show/Hide Whitespaces button + Add unit tests

    Compare with previous version

  • added 1 commit

    • c23c0f59 - Fix Show/Hide Whitespaces button + Add unit tests

    Compare with previous version

  • Thanks @leipert, I've addressed your comments, could you please have a look and assign to a maintainer?

  • LGTM! @iamphill Could you do the maintainer review?

  • assigned to @iamphill

  • Phil Hughes
  • Phil Hughes
  • assigned to @jerasmus

  • added 1 commit

    Compare with previous version

  • Thanks @iamphill please have another look.

  • Phil Hughes resolved all discussions

    resolved all discussions

  • Phil Hughes approved this merge request

    approved this merge request

  • merged

  • Phil Hughes mentioned in commit 05af98bf

    mentioned in commit 05af98bf

  • Please register or sign in to reply
    Loading