-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add warning if selected config has type = 'unknown'
#2533
base: main
Are you sure you want to change the base?
Conversation
@@ -80,6 +80,19 @@ | |||
>Edit the Configuration</a | |||
>. | |||
</p> | |||
|
|||
<p v-if="home.config.active.isUnknownType"> | |||
The selected Configuration has an unknown type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selected Configuration has an unknown type. | |
Please set the framework type: |
Something slightly more active + describing it as "framework type" might be more helpful for folks. The type hinting will tell them what the values are once they get there (so long as they have evenbettertoml installed), but maybe even tossing in an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And FWIW my suggestion there I'm not super excited about please do feel free to wordsmith
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The experience of manually updating a configuration from an "unknown" inspection result into a valid one is a rough road. If you haven't tried it, you can recreate the lovely experience by selecting a python file from a hello world example.
Building on what @jonkeane was suggesting, but also considering that we try to keep that text short when warnings/errors are displayed, perhaps you could display a helpful message with further explanation off of an anchor in the warning? I'm visualizing the link opening a modal notification in the center of the IDE with the additional directions, with an OK button to dismiss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps something like the below?
The selected Configuration has an unknown type. Edit the Configuration to specify the content type, for example:
type = 'quarto-shiny'
.
I kept the "The selected Configuration has..." language to keep this uniform with the other warnings we have in this area.
Same with the "Edit the Configuration" being the link to open the Configuration.
Would "content type" be preferable over "framework type" to get closer to Connect language? We aren't only specifying the content type since we have quarto-shiny
, rmd-shiny
, python-shiny
, etc, but figured that may be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy that the rewording addresses my concern about message size.
Would the use of content/framework type
address the concerns raised above?
My second concern was a more general one regarding the "unknown" content type and hand-creating valid configuration files, but that is out of scope for this PR.
Let me know when you've implemented changes and I'll take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would "content type" be preferable over "framework type" to get closer to Connect language? We aren't only specifying the content type since we have quarto-shiny, rmd-shiny, python-shiny, etc, but figured that may be better.
content
is something of bespoke jargon to Connect. It is an english word that means something that is close to the way we use it in Connect land, but in this context I don't think it's helping. IMO we should just talk about the framework not being known / needing to specify which framework they are expecting. How about something like "Use the framework that you're using as the type, ..."
I kept the "The selected Configuration has..." language to keep this uniform with the other warnings we have in this area.
I hear you on the consistency, but IMHO these are extra words that distract from what we want to communicate so eliminating them (even if it's less consistent and we aren't planning to undo the rest) is a better step forward, but if y'all think the consistency is important we can keep it. Similar to content
up above selected configuration
is something of publisher-specific jargon — sometimes we need to name concepts like that, but I don't think these warnings are places where we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic points. I'm convinced on the framework wording to get as close as we can to what users are expecting. Only a few of our types make that wording a bit awkward - like html
.
The alerts should be like calls-to-action and start with a verb — like your "use the framework..." example — so I agree that removing distractions and jargon to get to there is helpful.
I do want to include the link to open the configuration still which has me leaning into the "Edit the Configuration" verbiage to make very obvious - as opposed to making the whole thing or verb a link.
Edit the Configuration to set the framework you are using as the type, for example: type = 'quarto-shiny'.
"as the type" feels remove-able to keep this as short as possible.
Edit the Configuration to set the framework you are using, for example:
type = 'quarto-shiny'
.
Not opposed to the "Use the framework that you're using as the type, for example..." wording by the way. Where were you thinking about placing the link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do want to include the link to open the configuration still which has me leaning into the "Edit the Configuration" verbiage to make very obvious - as opposed to making the whole thing or verb a link.
Aaah sorry, I should have included this in my last comment, but edited it out, apparently (or thought it but it disappeared between head and keyboard 🤦 ). Agreed that having something like Edit the Configuration
as the anchor for the link is good, and very much agree that having it something it not be the whole message is 💯 . What about:
Please set the framework you are using, for example:
type = 'python-shiny'
. Edit the Configuration
I like the example you added there (I thought it might be too extraneous, but it helps concisely show that the key is type
and here's on example that you might know). I changed it to python-shiny
but we could debate or use another framework. This is outside of this PR but I wish that we could use simply quarto
here. For many many many folks quarto-static
is the only kind of quarto
there is, and seeing quarto-static
is just confusing. quarto-shiny
exists, but isn't super popular / is for a pretty specific set of circumstances. I picked python-shiny
here since it reps Posit frameworks and is comforting to python data scientists. But one could make good arguments that using quarto-static
, r-shiny
, python-bokeh
, or python-streamlit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please set the framework you are using, for example: type = 'python-shiny'. Edit the Configuration
Fantastic. I'll use this. I really like the reasoning on how you came to the type example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7e8379d
to
2801428
Compare
03e1402
to
0f48868
Compare
This PR adds a new warning if the selected configuration has
type = 'unknown'
now that it is a valid type in the schema due to the work in #2423.Preview
I'm very open to suggestions regarding the text for the warning.
Intent
Resolves #2515
Type of Change
User Impact
When a selected configuration has
type = 'unknown'
the user will now see a helpful warning (similar to our others like duplicate environment variable names warning).This can occur either through a manual change, but also if inspection isn't able to reason about the
type
and the initial configuration defaults totype = 'unknown'
.