Refactor SaveBar with hide Show button

[megcrenshaw]

Hides the "show" button on the Lesson Edit page if there's no lesson plan. On the Lesson Edit side, I checked for a lesson plan inside initialLessonData––this doesn't ensure the path actually works.

On the SaveBar side, I did the following:

1. Provided a default handleView to use if a path is given (default does not use linkWithQueryParams)
2. Allow a custom handleView to be passed in––uses that if given.
3. Hides the Show button if a path isn't given and if no handleView is given.
4. Refactored component to use DefaultProps and be a function component.

I updated CourseEditor, LessonEditor, and UnitEditor to take into account these changes. I didn't change ProgrammingExpressionsEditor.

I suppose it's possible someone will want to show the button even if they don't pass in a function or a path. Do we want to allow more control to show/hide it?

Links

• jira ticket: PLAT-1435

Testing story

I tested manually and added/refactored some unit tests.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[megcrenshaw]

This was to fix a warning about the select having an empty value

[davidsbailey]

TIL about defaultProps! I read about these and I have a couple of questions/ideas for best practices:

1. is there any reason to include error and handleView whose defaults are undefined? this seems like it would have no effect. if not I'd suggest removing those lines.
2. propType resolution happens after defaultProps are applied, so props with an entry in defaultProps could be able to be marked as isRequired. I think our current practice is to use isRequired whenever possible, but others feel free to correct me here

[megcrenshaw]

Yeah, good questions. I suppose we should norm on best practices, and it sounds like there already is a norm for isRequired. I am accustomed to (a) only marking isRequired for props that folks using the component need to provide, and then (b) providing defaults for all not-required props so that people can easily tell what's happening by default. I suppose I don't feel strongly about (b), but feel more strongly about (a): it feels like marking isRequired for all of these, but then only having an error if a person doesn't provide handleSave, could be confusing. Then again, we have tests that pretty clearly show that, and it sounds like that's a different norm from what we're already doing.

UPDATE: I did some research, and perhaps I'm not pulling (a) out of thin air: Someone on this Stack Overflow post writes, "The way it's usually thought of is the component is guaranteed to render without errors if all the required props are supplied. The props that are not required would only enhance the component with additional functionality."

[davidsbailey]

nice, I like that better - it does seem more useful that the isRequired props would only be the ones the caller has to provide. let's go with that!

[megcrenshaw]

@davidsbailey, are you thinking I should remove the undefined defaultProps? I'm good to do so––curious to hear what you think.

[davidsbailey]

this looks like it might be going against the premise of "no special values". is there a case where this '/' is getting used? I would expect there not to be, since my understanding of the new business logic is that we don't want to use this value if it is missing, and it looks like the programming expressions editor uses its own handleView method.

If that's correct, I would suggest letting pathForShowButton be undefined by default, and then just testing || pathForShowButton or similar here.

if that all works... then you could also get rid of handleView and just have the ProgrammingExpressionEditor pass in pathForShowButton: '/', if that's the logic we still want there.

maybe @bethanyaconnor you can confirm what the best behavior is for the show button in ProgrammingExpressionEditor?

[megcrenshaw]

Yeah, I think that's a great call –– also, then if people want a show button but don't want it to go anywhere, they can use '/' for the path to get it to show up. Bethany, would still love to hear your ideas!

UPDATE: The "/" for the path causes the redirect to go to the home page since we aren't using linkWithQueryParams, so the change is quite good. I realized that navigateToHref(pathForShowButton) where pathForShowButton might be undefined is a little wonky, even if onClick shouldn't run in that case because the button won't be visible. Still, I made an anonymous function to fall back to. Could also make the button disabled if it's not visible?

[davidsbailey]

this is an interesting situation, where we're hiding the button but we're still trying to handle what happens if something calls the click handler anyway. I don't have a great example to offer, other than that we have typically preferred conditional rendering and so this problem doesn't come up. there's a lack of clear advice on the internet about when to use conditional rendering, but I would say our current most common practice is to prefer conditional rendering, unless you are optimizing for an expensive component that changes visibility a lot in which case using css to hide the component is cheaper.

I don't feel super strongly about preferring conditional rendering over css visibility per se, but if it would make it clearer that we could never hit the click handler when the component is hidden then it could be a good option in this case.

[davidsbailey]

if you do want to keep using css visibility then I would argue for NOT taking extra steps to handle the click in the case where the element is hidden -- if we think that hiding the css element doesn't really convince us that its not clickable then I would take that to mean that conditional rendering would be better.

[megcrenshaw]

Good call––I was using visibility: hidden to be sure I wasn't causing unintentional styling changes. I've changed it to render an empty <span \> which is perhaps clunky, but (a) doesn't render a button if we don't need it, and (b) avoids the other buttons moving around.

[bethanyaconnor]

FYI for ProgrammingExpressionEditor the / path is being replaced in https://github.com/code-dot-org/code-dot-org/pull/43659/files, which I hope to merge today

[megcrenshaw]

Excellent! I didn't want to change anything in that file because I knew you were updating it

[davidsbailey]

Good call––I was using visibility: hidden to be sure I wasn't causing unintentional styling changes. I've changed it to render an empty <span > which is perhaps clunky, but (a) doesn't render a button if we don't need it, and (b) avoids the other buttons moving around.

Ahh, yes, that is a very good reason to use css visibility which I had not considered. empty span sounds like a winner to me. thanks for all the thoughtful conversation here!

[davidsbailey]

LGTM after considering the follow-up on the button visibility.