feat: show hidden fields in history frontend

remidej commented 2 weeks ago

What does it do?

Makes hidden fields visible in the History page. This is not about internal fields like id, but about the ones the user may have deleted from the layout via the configure the view page. I called them "remaining fields" in the code, to match what was already done in the code for that page.

It applies the fix in two places:

At the root of the history page layout. This one was the most straight forward.
Inside the components. We didn't have direct access to the component's rendering logic, so I had to refactor it to pass a new renderLayout. It has the benefit of avoiding duplicating logic between repeatable and non-repeatable components, and avoids some prop drilling for renderInput.

I want to write e2e tests for this, they have issues we need to fix first.

Why is it needed?

Because even though they're not visible in the edit view, the fields that aren't in the layout would still be restored if the history version is restored. It's therefore important to show them regardless, so that users know what they're restoring.

How to test it?

Go to the configure the view page for a content type. Delete a root field there (not in the CTB), as well as a field that's inside a component.

Now open the history page for an entry of that content type. You should see the deleted root field on a new line at the bottom of the page, but above the unknown fields.

Also check that component inputs work as expected in the Edit View page, since their code was refactored.

vercel[bot] commented 2 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
contributor-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 30, 2024 3:59pm

remidej commented 1 week ago

@joshuaellis I think I see what you're saying, but with the current state solution I think I'm happy where things stand:

HistoryPage fetches the layout data. I like that because that component centralizes all the loading and error states, so we have one single Loader
VersionContent transforms the layout and renders it. It's the only purpose of the component so I feel like it's pretty readable
VersionInputRenderer does a little big of logic to compute the layout for components

For components I agree it could be nice to not have some layout logic in the input renderer for components. But I feel like extracting things to a hook will add more complexity rather than simplify things 🤔

remidej commented 1 week ago

@markkaylor thanks for bug report! I was only checking with content types that had hidden fields.

About the hidden fields section. I thought about that but I'm not sure it's a good idea. I think it's important that unkown fields remain clearly different from the rest, because they're the ones that actually behave differently, as they can't be restored. If we have three different sections, we lose a bit of the clear page distinction between restorable and non-restorable. So I'm afraid trying to over-explain things by would actually create confusion.

There's also the issue of hidden fields inside of components. You can't extract them into their own section of the page, since they would lose their context. So you'd have to split each component into a known and an unknown section. It's not something I'm excited about.

We can also check with Lucas when he's back, but he's OOO until the Barcelona

remidej commented 1 week ago

I found one last error when finishing my QA when trying to access history of a content type that does not have a component:

@markkaylor I can't reproduce this, could you tell me which content type you tested with? I think tried content types with and without components, with and without data in them, with and without hidden fields

I noticed if I hide a field in a component via configure the view, then in the EditView it's hidden but in the history version it's just still there in the component. On the other hand if I hid the entire component via configure the view it shows up in the panel with the other hidden fields as expected. I guess that's the kind of thing you are talking about here and that's tomorrow's problem?

I believe both cases are what we want, what is the issue you're seeing? Seeing a hidden field within a component is the reason I'm doing this whole refactoring. And for the hidden component there's no other choice but to show it at the bottom

markkaylor commented 1 week ago

So for components, I can hide a component and it works as expected:

https://github.com/strapi/strapi/assets/26598053/89bf46f4-c15d-4216-8d02-f0fe81e09fe5

But if I try and hide a field inside a component, then in the EditView it's hidden but in history it is not represented as a hidden field.

https://github.com/strapi/strapi/assets/26598053/3a037999-3ddd-4a4e-9429-74db40a44c14

Let me know if it's clear or if you prefer to take a look on discord or something.

For the error, I had it on the Tag content type but I am unable to reproduce now too. Maybe it was stale data or something 🤷‍♂️

strapi / strapi