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

Remove TitlePart wrapper when it is hidden #15471

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

douwinga
Copy link
Contributor

@douwinga douwinga commented Mar 7, 2024

Fixes #15382

@sebastienros
Copy link
Member

@douwinga PR is fine, did you actually face an issue with this empty wrapper? Or did you find the ticket and wanted to help fix it.

@MikeAlhayek MikeAlhayek enabled auto-merge (squash) March 7, 2024 21:57
@MikeAlhayek MikeAlhayek merged commit d069eb2 into OrchardCMS:main Mar 7, 2024
4 checks passed
@douwinga
Copy link
Contributor Author

douwinga commented Mar 7, 2024

@sebastienros Just trying to contribute to become a better developer. Was looking for a ticket I could finish on my lunch break. This was the first one I found.

@sebastienros
Copy link
Member

So we are hoping this doesn't break anything, the issue wasn't to fix an actual problem but something that looked odd. It's totally possible that some other feature was using (or could use in the future) these wrappers even if the shape was not rendered (e.g. content preview, placement, shape debugger, ...)

@douwinga
Copy link
Contributor Author

douwinga commented Mar 7, 2024

Hmmm, I bet this change would actually cause an issue with admin templates and shape rendering. It would no longer obey the hide check. Should probably keep that check on the TitlePart.Edit. it seemed like an innocent change

@sebastienros
Copy link
Member

If you can do some test and see if nothing is broken, otherwise we'll just revert the change.

@douwinga
Copy link
Contributor Author

douwinga commented Mar 7, 2024

Ya, I will look tonight and try to fix any issues. Having an empty div isn't ideal. I wonder if these wrappers were just to give JavaScript something to target or something.

@sebastienros
Copy link
Member

I wonder if these wrappers were just to give JavaScript something to target or something.

Definitely. Question is do these wrappers still like to have the div even if the part is not displayed ... I would say that if the wrapper doesn't bother, we should just keep it.

@MikeAlhayek
Copy link
Member

I tested the change before merging and it worked as expected with no issues. But another set of eyes wont hurt.

@douwinga
Copy link
Contributor Author

douwinga commented Mar 8, 2024

I was wrong. Having {{ Model.Parts.TitlePart | shape_render }} in an admin template does still obey the hidden setting. So far nothing else has broken either.

@douwinga
Copy link
Contributor Author

douwinga commented Mar 8, 2024

It breaks with a placement.json with the following:

{
  "TitlePart_Edit": [
    {
      "place": "Parts:0",
      "displayType": "Edit"
    }
  ]
}

If we do the TitlePartOptions.GeneratedHidden check in TitlePart.Edit.cshtml it works correctly as the only thing that is broken is that it no longer is checking that and shows when it should be hidden.

I can do a PR to just have it check in TitlePart.Edit.cshtml too and not just ContentPart-TitlePart.Edit if we think we are not just going to revert this change.

@douwinga
Copy link
Contributor Author

douwinga commented Mar 8, 2024

shape debugger

Not sure what that is to test

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

Successfully merging this pull request may close these issues.

Remove TitlePart wrapper when it is hidden
3 participants