OS-81/project-permissions-render-on-tabhide

[brentguf]

No description provided.

[nTraum]

Hi there, I've overwritten your branch (and all others) to get rid of dangling secrets in this repository. Therefore please run:

git pull --all -X theirs 

before you start working on it again. It will pull all your locally tracked branches and overwrites them with what we have on GitHub.
Your diff should now look the same as yesterday evening when you finished work.
In the unlikely event of having issues, please reach out to me and we'll resolve it together. I backed everything up at 06:15 AM CEST, so nothing will be lost if you pushed your work yesterday evening.
The production branch is unfortunately still dangling, I couldn't overwrite it because of GitHub branch protections. Will do this later today, for now please do not merge into or from this branch. I will update you in this thread again once this is resolved.

[geoffreydhuyvetters]

Bit confused by this PR, isn't this the same check twice?

[brentguf]

Bit confused by this PR, isn't this the same check twice?

Hm yes, not sure yet if I'm going to continue with this one. The idea was: actually what was RenderOnFeatureFlag should have been the tabHideCondition as we can see for other tabs in project/edit/index. For this tab, the hide conditions happens to be the feature flag, so I was thinking of making it clear there's a difference and tabHideCondition might diverge from the feature flag in the future. But the feature flag should also stay there, I think.

Clearer?

[geoffreydhuyvetters]

I don't completely get why this would be clearer?

only render this component when the feature flag is enabled = hide the tab when it's disabled

[brentguf]

I don't completely get why this would be clearer?

only render this component when the feature flag is enabled = hide the tab when it's disabled

In the future the condition for hiding the tab might be 'when the participation method is not ideation' for example. But we still only want to render it then when the feature flag is also enabled. But the feature flag might not be equal to the tabHideCondition at that point (which is now the case).

Whether something is enabled or not can be because of the pricing plan a platform is in, which might be different from the reason we want to show/hide it, which can be due to the format of the project.

[geoffreydhuyvetters]

I wouldn't code for future scenarios, for now, this tab should be hidden when the feature flag is disabled. We can always refactor and rename it later when this would be the case.

But that's just my opinion.

[brentguf]

I wouldn't code for future scenarios, for now, this tab should be hidden when the feature flag is disabled. We can always refactor and rename it later when this would be the case.

But that's just my opinion.

I agree, but this is not coming from nowhere. It's because this used to be called tabHideCondition before it was extracted (see front\app\containers\Admin\projects\edit\index.tsx file for remniscants). And that might not be clear otherwise. If you search the codebase for RenderOnHideTabCondition, you can see how I did it for other tabs. For these tabs, it's the same, but it happens to be exactly the feature flag, which makes it less obvious imo.

[brentguf]

We first passed the tabHideCondition as part of onData's argument, but undid that, because that spelled future trouble when you have 4 insertions on the same page and then the condition of the first insertion would be that of the last insertion, because it would get overwritten on every tab insertion. I can show some code if my text doesn't make sense.

Edit: you can see here (https://github.com/CitizenLabDotCo/citizenlab/pull/48/files) how it was done initially. And so tabHideConditions would get overwritten if you insert the same tab from 4 different spots as we do with the permissions tab.

[geoffreydhuyvetters]

Ok, whatever makes the most sense in the bigger context then.

I still think it's should just be a rename of the original wrapper component then, no need to have it twice.
We can always expand up the wrapper if we want other conditions later.

[brentguf]

Ok, whatever makes the most sense in the bigger context then.

I still think it's should just be a rename of the original wrapper component then, no need to have it twice.
We can always expand up the wrapper if we want other conditions later.

Then it should be the RenderOnTabHideCondition or whatever I've called it, instead of the RenderOnFeatureFlag wrapper. :)