DOP-4623: [Dark mode updates] Cards component + card icon as string #601
Conversation
mayaraman19
changed the title
mayaraman19
requested review from
i80and,
seungpark,
rayangler,
mmeigs and
caesarbell
| @@ -118,32 +118,6 @@ class LegacyTabsDefinition(nodes.Node): | |||
| tabs: List[LegacyTabDefinition] | |||
|
|
|||
|
|
|||
| @checked | |||
There was a problem hiding this comment.
all this removal brings me great joy!
snooty/diagnostics.py
Outdated
| end: Union[None, int, Tuple[int, int]] = None, | ||
| ) -> None: | ||
| super().__init__( | ||
| f"""Card icons are migrating to strings instead of path names. If {image_argument} can be a string, use that instead.""", |
There was a problem hiding this comment.
(Non-blocking, but maybe worth discussing) Would it be worth including more information on what the "string" is supposed to be or how to know what the string is supposed to be?
Also, I know that we have a strategy for handling marketing's icons through the web assets URL, but do we have a strategy for other icons like for drivers/languages/integrations? If we don't currently support all existing icons by using strings, I'm afraid of causing noise and confusion through this warning for card icons that can't immediately be updated. Should we consider adding this warning at a later time once we fully support everything?
There was a problem hiding this comment.
that sounds good!
There was a problem hiding this comment.
i am wondering if the warning that a dark mode icon hasn't been included should be silenced until we are closer to releasing dark mode?
There was a problem hiding this comment.
I think I'm okay with waiting if we need the time to sort out all of the dark mode icons for cards
There was a problem hiding this comment.
Removed the warning!
| @@ -118,32 +118,6 @@ class LegacyTabsDefinition(nodes.Node): | |||
| tabs: List[LegacyTabDefinition] | |||
|
|
|||
|
|
|||
| @checked | |||
snooty/parser.py
Outdated
| # for cards - if the image is a path, we need a dark mode version as well | ||
| if key == "mongodb:card": | ||
| if image_argument and "/" in image_argument: | ||
| self.diagnostics.append(CardIconString(image_argument, line)) |
There was a problem hiding this comment.
similar to @rayangler s comment above but wouldn't this PR still have a warning ?
think we should allow name and path, but require a icon-dark if its a path
snooty/parser.py
Outdated
| dark_mode_image_argument = options.get("icon-dark") | ||
| if dark_mode_image_argument: | ||
| self.validate_and_add_asset(doc, dark_mode_image_argument, line) | ||
| else: |
There was a problem hiding this comment.
nit: Collapse this into a single elif image_argument:
Ticket
DOP-4623
Notes
Ugh, there's a lot going on in my brain for this but hopefully I get everything.
icon-darkoption has been added to cards, as well as a warning ificoncomes from an image stored in the content repo buticon-darkis not given.BaseCardGroupDirective,CardDefinition, andCardGroupDefinitionclasses 🎉Example parser output now looks like this (for the docs-compass PR linked in the Snooty PR):
Relevant Snooty PR here.
Some future work here can involve:
README updates