Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

fix(StatusPopupMenu)!: Refactoring #894

Open
wants to merge 1 commit into
base: fix/status-menu
Choose a base branch
from

Conversation

igor-sirotin
Copy link
Contributor

@igor-sirotin igor-sirotin commented Sep 9, 2022

This PR is for status-im/status-desktop#7232

  1. All Menu components are refactored.

  2. Some UI improvements made:

  • Highlight menu when submenu opened
  • Proper colors for disabled menu items
  1. I'm also planning to rename the components as following in a separate PR after testing with status-desktop
Old name New name
StatusPopupMenu StatusMenu
StatusMenuItem StatusAction
StatusMenuItemDelegate StatusMenuItem

Checklist

  • follow conventional commits
    • the scope should be the component's name e.g: feat(StatusListItem): ...
    • when adding new components, the scope is the module e.g: feat(StatusQ.Controls): ...
  • add documentation if necessary (new component, new feature)
  • update sandbox app
    • in case of new component, add new component page
    • in case of new features, add variation to existing component page
    • nice to have: add it to the demo application as well
  • test changes in both light and dark theme?
  • is this a breaking change?
    • use the dedicated BREAKING CHANGE commit message section
    • resolve breaking changes in status-desktop
      • (pre-merge) adapt code to breaking changes
      • (post-merge) update StatusQ submodule pointer
  • test changes in status-desktop

@igor-sirotin igor-sirotin changed the title Fix/issue 7232 fix(StatusPopupMenu)!: Refactoring Sep 9, 2022
Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

Just gave it a quick glance. There's a bunch of comments left in the code base.
Is this a WIP?

Also, this needs to be tested a lot also inside desktop as these changes are pretty invasive.
Would it be possible otherwise, to break this up in to smaller pieces?

Say, first make StatusMenuItem a StatusAction, then get those changes into Status Desktop, then make StatusPopupMenu a StatusMenu etc?

It's just one of those breaking changes that will shoot us massively in the foot.

Another proposal: Introduce StatusAction, ... etc in addition to the existing compnents, then replace legacy components in desktop, then get rid of legacy components in StatusQ

src/StatusQ/Popups/StatusSearchLocationMenu.qml Outdated Show resolved Hide resolved
src/StatusQ/Popups/StatusMenuItemDelegate.qml Outdated Show resolved Hide resolved
sandbox/pages/StatusPopupMenuPage.qml Outdated Show resolved Hide resolved
@igor-sirotin
Copy link
Contributor Author

@PascalPrecht, actually I am making the changes partially - this is a PR to fix/status-menu.
After merging this, I will also make required changes in status-desktop, then probably make some fine minor tunings in StatusQ and then merge everything together.

About partial changes file-by-file: it's also not possible, because there are property interdependencies, which are removed.

I thought about making a "temporary-naming" transition, It's not possible cause we rename current StatusMenuItem and also rename StatusMenuItemDelegate to StatusMenuItem, so there is a conflict.
Though, I can make some names with prefix like _2 and then rename them, but I think an intermediate branch fix/status-menu will help here.

Sorry, I will remove the comments.
Think of them like there're none 🙂

@caybro
Copy link
Member

caybro commented Sep 15, 2022

@igor-sirotin looks like a rebase gone wrong... all the chart stuff and extra commits in this PR :)

@igor-sirotin
Copy link
Contributor Author

@caybro that was strange! Fixed now 👌

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants