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

[HOLD MERGE] Update diff view for forthcoming Watchlist feature #4552

Merged
merged 18 commits into from
Jul 5, 2023

Conversation

staykids
Copy link
Contributor

Phabricator: https://phabricator.wikimedia.org/T335579 (partially)

Notes

Updates diff view to new design in support of forthcoming Watchlist feature. I've retained the single revision view instead of removing it entirely mostly as a safety net, as there may be special cases or entry points where it still makes sense to see a single revision (like an article that only has a single revision).

Ready for review - feel free to merge if you feel it's ready to be merged, or otherwise wait for forthcoming stacked PRs before merging into main.

Remaining work to come in future stacked PRs includes:

  • Continuing design tweaks (some awaiting feedback from design in Phab task)
  • Showing article image if available in header
  • Improving next/previous revisions handling around boundaries of first revisions
  • Updating header itself and header button design to not be full width
  • Update Components framework menu button to support post initialization configuration (to handle our client pattern of lazy view configuration)
  • Accessibility pass of the new work
  • Hook up functionality of new menu buttons and header user buttons

Test Steps

  1. Open an article and view the edit history
  2. Tap a single revision and confirm you see the updated design in the "compare" two revisions display
  3. Confirm design update of main content, new toolbar icons and actions, and updated header
  4. On a device with a large width (iPad), confirm new header author/edit details show side by side instead of stacked

Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

Looks good! I'm going to go ahead and merge. I think that will be easier to manage moving forward than stacked PRs. Note I did a few minor commits at the end (updating with main, turning the feature flag on).

lazy var moreButton: IconBarButtonItem = {
// DIFFTODO: Add menu item images
let menu = UIMenu(title: "", options: .displayInline, children: [
UIAction(title: CommonStrings.rollback, attributes: [.destructive], handler: { [weak self] _ in self?.tappedRollback() }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FYI, visibility of the rollback option may need to changed based on permissions data from the API. I can look into this once the API calls are merged.

@tonisevener tonisevener merged commit 2ddbd8d into main Jul 5, 2023
2 checks passed
@tonisevener tonisevener deleted the T335579-updated-diffs branch July 5, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants