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

Fixing publish descendants dialog #8601

Merged
merged 2 commits into from
Aug 7, 2020
Merged

Conversation

clausjensen
Copy link
Contributor

@clausjensen clausjensen commented Aug 6, 2020

Description

The publish descendants dialog was broken at some point going from 8.6->8.7.

I've fixed it up, using the label on the toggle directive instead of using a separate label.

Also updated the localized texts to better reflect what is happening here:

  • The toggle allows you to include or exclude publishing of items that have a published state.
  • The old help text could indicate that items in a published state but with a "draft" on top would not be published if deselected ... they would be - so this is now more clear.

The last fix done here is that the selected value would not be pushed back to the scope which means your selection would never make it back.


This item has been added to our backlog AB#7682

updating localized texts to reflect what is happening.
ensure the selected value is pushed back to scope and used for the action.
@clausjensen
Copy link
Contributor Author

When merged, should also be cherrypicked for 8.7 as it is a regression.

@clausjensen clausjensen added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Aug 6, 2020
label-off="{{vm.labels.includeUnpublished}}"
label-position="right"
show-labels="true"/>

Copy link
Contributor

@bjarnef bjarnef Aug 6, 2020

Choose a reason for hiding this comment

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

@clausjensen notice angular directives/components can't be void elements (self-closing), so it needs a end-closing </umb-toggle>.

There is a great explanation of it here: https://stackoverflow.com/a/25012451/1693918

Also here is an example of what can happen when a component/directive is self-closed. Of course native HTML5 elements can still be self-closed like <img />, <br /> etc.
#8478

Here is a PR updating this for <umb-checkbox> and <umb-radiobutton>:
#8547

and this for <umb-toggle> #8605
However if merging this PR and including end-closing </umb-toggle>, then #8605 can just be closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've merged your PR as it might be good just for history .. and the whitespace change is fine. Merging this afterwards will overwrite some things in yours which is fine.

@bjarnef
Copy link
Contributor

bjarnef commented Aug 6, 2020

@clausjensen I have submitted the following PR: #8605

But it seems it was only in this overlay <umb-toggle> was void element. In the PR I removed some whitespace where toggle was used in macros, but since you have localized the text in the controller it should be fine and don't need the <localize> directive in view. It is just missing the end-closing </umb-toggle> instead.

@clausjensen clausjensen changed the base branch from v8/dev to v8/contrib August 7, 2020 07:49
@clausjensen clausjensen changed the base branch from v8/contrib to v8/dev August 7, 2020 07:49
@clausjensen clausjensen merged commit 39fcdb0 into v8/dev Aug 7, 2020
@clausjensen clausjensen deleted the v8/bugfix/publishdescendants branch August 7, 2020 07:56
@umbrabot umbrabot removed the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Aug 7, 2020
@clausjensen
Copy link
Contributor Author

I've removed version tag on this as I'm fairly sure this was never actually released in a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/ui User interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants