Skip to content
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

revert refresh button #19522

Merged
merged 9 commits into from Jul 25, 2022
Merged

revert refresh button #19522

merged 9 commits into from Jul 25, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jul 13, 2022

Description:

Fixes: #19515
revert refresh button

Review

@sgiehl
Copy link
Member

sgiehl commented Jul 18, 2022

I would personally at least keep the keyboard shortcut. But that might be not a decision I'm allowed to make.

@tsteur
Copy link
Member

tsteur commented Jul 18, 2022

Keyboard shortcut be good to have indeed

plugins/CoreHome/javascripts/top_controls.js Outdated Show resolved Hide resolved
plugins/CoreHome/lang/en.json Show resolved Hide resolved
plugins/CoreHome/CoreHome.php Show resolved Hide resolved
@sgiehl
Copy link
Member

sgiehl commented Jul 20, 2022

@justinvelluppillai should this one go into 4.11 ? Otherwise the button would be added with 4.11. and removed again with 4.12. Which imho wouldn't be good.

@sgiehl sgiehl changed the title revert refresh bottom revert refresh button Jul 20, 2022
@justinvelluppillai justinvelluppillai changed the base branch from 4.x-dev to next_release July 21, 2022 05:50
@justinvelluppillai
Copy link
Contributor

@sgiehl yes that is the intent here. @peterhashair this would need to be rebased of the next_release branch now.

Peter and others added 5 commits July 21, 2022 13:41
@sgiehl
Copy link
Member

sgiehl commented Jul 21, 2022

@peterhashair I have rebased the branch. Might be possible some of the screenshots might need another update due to the change.
Also I had added a couple of comments regarding still required translations in my last review. You have marked them as resolved, but haven't pushed any further update. It would be helpful to only mark review comments as resolved if you

  • either pushed something that really fixes it
  • or add a comment that really resolves it (like answering a question)

Or what might be an even better handling: Give feedback on each comment like "I've pushed a fix", answer questions or ask a question if it wasn't clear. Afterwards the reviewer can resolve the comments himself in the next review round.

update screenshots and add translation
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

I have looked through all the screenshots you have already updated. It seems the changes on plugins/Transitions/tests/UI/expected-screenshots/Transitions_transitions_popup_urls.png are not caused by changes of this PR, so you should avoid updating it.
Also tests/UI/expected-screenshots/UIIntegrationTest_shortcuts.png seems not be updated to the latest version, as it still lacks the refresh shortcut.

I've now seen that a couple of times on your PRs that screenshots are updated, where they shouldn't have been. In general you should avoid simply using the sync command to download all screenshots and afterwards add all changes and push them. It is mandatory to look through all changes on the artifacts server before and validate that the changes on a screenshot are caused by your changes. Even if changes in a screenshot look valid, avoid updating it in a PR that didn't cause the changes.

plugins/CoreHome/CoreHome.php Outdated Show resolved Hide resolved
Peter Zhang and others added 3 commits July 25, 2022 11:26
Co-authored-by: Stefan Giehl <stefan@matomo.org>
update shortcut screenshot and revert Transition screenshots
@peterhashair
Copy link
Contributor Author

@sgiehl will pay more attention to the screenshots updates from now on 👍. Updated it.

@sgiehl sgiehl added this to the 4.11.0 milestone Jul 25, 2022
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 25, 2022
@sgiehl sgiehl merged commit f30bf4f into next_release Jul 25, 2022
@sgiehl sgiehl deleted the m19515 branch July 25, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove again the "refresh" icon
4 participants