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

Modify hs create module command to correspondence to content_types (Previously host_template_types) #1225

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kotto5
Copy link

@kotto5 kotto5 commented Oct 27, 2024

Description and Context

Modify hs create module command's content_type checkbox to choose value for content_types.
Previously, you could choose a value for host_template_types.
It causes an error when you upload modules.

#1224

Screenshots

Before
Screenshot 2024-10-27 at 23 38 03

After
Screenshot 2024-10-27 at 23 36 36

After version may have problem about UI. Choices are too many to choose and to display at once so you have to scroll to see all choices. It can make confuse user who create modules.

TODO

  • Discussion about UI and improvement may be needed.

Who to Notify

@kotto5
Copy link
Author

kotto5 commented Nov 4, 2024

@graysky @also @timmfin @bbeaudreault
Is this PR unimportant?

@TanyaScales
Copy link
Member

@kotto5 - RE: this comment "After version may have problem about UI" and what to do here. I would be fine with this update moving through independently of UI updates to make this better (for the short term). We can discuss the proper pattern for long-form option lists over here on our side and make sure it is consistent across the commands.

@@ -30,10 +30,18 @@ const CONTENT_TYPES_PROMPT = {
message: i18n(`${i18nKey}.selectContentType`),
default: ['PAGE'],
Copy link
Member

Choose a reason for hiding this comment

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

Hi @kotto5 - one quick note that we should update the default here to point to the ANY option instead to support this change

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for review @TanyaScales

I fixed it based on your point, and I'm glad to see that the UI thing is not a problem either.
Hope you will check the fixes.

Copy link
Author

Choose a reason for hiding this comment

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

@TanyaScales
How do I ask other reviewers for a review after I have been approved by you?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @kotto5 - I brought this up internally with some of the others already tagged on this issue to discuss when it should be merged and released. So you should actually be all set here. We'll take care of the merge when we're ready for it to be lined up in the queue to go out.

Thanks for your contribution.

@kotto5 kotto5 force-pushed the modify_module_create_checkbox branch from e17ba1e to c4eb494 Compare November 20, 2024 13:32
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.

2 participants