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

Add views dropdown and footer actions to the "move to view" dialog #10172

Merged
merged 11 commits into from Oct 18, 2021
Merged

Add views dropdown and footer actions to the "move to view" dialog #10172

merged 11 commits into from Oct 18, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 6, 2021

Breaking change

Proposed change

Before:
image

After:
image

Closes #10083

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@homeassistant
Copy link
Contributor

Hi @MartinTuroci,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@spacegaier spacegaier changed the title feat: add views dropdown and footer actions to the move dialog Add views dropdown and footer actions to the "move to view" dialog Oct 6, 2021
@homeassistant
Copy link
Contributor

Hi @MartinTuroci,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@bramkragten
Copy link
Member

I'm not a big fan of this UX personally, the old UI could definitely use some clarification, but I feel like having to open another dropdown doesn't really help...

I think the buttons are a good idea, but lets keep a list, and add a selection indicator.

Let's wait for the verdict of @matthiasdebaat on this :-)

@matthiasdebaat
Copy link
Collaborator

matthiasdebaat commented Oct 7, 2021

Good that you mentioned how it looks like when there are more views.

This is how it looks right now.
image

My proposal is to use radiobuttons.
Dialog

@ghost
Copy link
Author

ghost commented Oct 7, 2021

Will do @matthiasdebaat

@bramkragten
Copy link
Member

Let's only do that if there is more than 1 view btw :-)

@ghost
Copy link
Author

ghost commented Oct 9, 2021

Let's only do that if there is more than 1 view btw :-)

@bramkragten What should be the fallback for a single view then? Non-clickable non-radio list item?

@ghost
Copy link
Author

ghost commented Oct 9, 2021

All review comments addressed, current state:
image

Waiting for clearance about single view edge case.

@bramkragten
Copy link
Member

Let's only do that if there is more than 1 view btw :-)

@bramkragten What should be the fallback for a single view then? Non-clickable non-radio list item?

Or just don't render the view list at all? If there is 1 view you basically just move it to that dashboard? @matthiasdebaat ?

@matthiasdebaat
Copy link
Collaborator

Or just don't render the view list at all? If there is 1 view you basically just move it to that dashboard? @matthiasdebaat ?

Yeah, I agree

@ghost
Copy link
Author

ghost commented Oct 11, 2021

@matthiasdebaat so if there is only 1 view I should display a dialog with dashboard dropdown only?

@zsarnett
Copy link
Contributor

I think we should keep the UI consistent even if there is only one...

Just make it checked. Not really a need to code around this case

@ghost
Copy link
Author

ghost commented Oct 11, 2021

I agree with @zsarnett. Having implicit option checked is probably better so that user knows what's going on.

@matthiasdebaat
Copy link
Collaborator

I think we should keep the UI consistent even if there is only one...

Just make it checked. Not really a need to code around this case

When there's only one view, there aren't tabs on the lovelace dashboard am I right? In that case it would be easier and more coherent to display a dialog with only a dashboard dropdown. When a users select a dashboard with multiple views it shows the different views.

@zsarnett
Copy link
Contributor

There still is a view. It's just hidden from the header when there is only one.

@bramkragten
Copy link
Member

There still is a view. It's just hidden from the header when there is only one.

Yes, but I don't see value in showing that to the user? Just like we dont show to the tabs when there is just 1 view?

@ghost
Copy link
Author

ghost commented Oct 12, 2021

Changed to show views only for > 1.

@ghost ghost requested a review from bramkragten October 12, 2021 13:21
bramkragten
bramkragten previously approved these changes Oct 12, 2021
Copy link
Member

@bramkragten bramkragten 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! 1 small remark

@ghost ghost requested a review from bramkragten October 17, 2021 19:01
@bramkragten bramkragten enabled auto-merge (squash) October 18, 2021 20:17
@bramkragten bramkragten merged commit 403c042 into home-assistant:dev Oct 18, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog to move a card to another Lovelace dashboard has an inconstant button
5 participants