-
Notifications
You must be signed in to change notification settings - Fork 1
Fix showing logic for notices, add compact style #107
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
base: master
Are you sure you want to change the base?
Conversation
Possibly convert to draft, I still want to add an example for the compact style but need the fixes from master for that |
<section class="description" v-if="stac.description || stac.deprecated || supportedBy || affectedByMissing"> | ||
<h3>Description</h3> | ||
|
||
<Description :description="stac.description"></Description> | ||
<Description v-if="stac.description" :description="stac.description"></Description> |
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 Description header is shown even if no description is shown, may also apply to other components.
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.
Yes that's correct, for all the changed components. In my stump this happens e.g. for the service types in the ServerInfo modal. Looks like this:
I didn't find that overly inappropriate, so I left it as it is.
But would you prefer to show the heading only if there really is a description? Could easily be achieved with a v-if="stac.description"
on the <h3>
.
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.
Yes, I think we can skip the description header in those cases. The proposed solution sounds reasonable.
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.
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.
That's on the edge, both options seem fine for me. Implement your preferred way ;-)
1a651aa
to
616ea05
Compare
Generally looks good, thanks. I rebased the PR to include the main changes and left one comment with regards to the Description heading. |
No description provided.