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

feat: Article card #1027 #1028

Merged
merged 8 commits into from Oct 21, 2021
Merged

feat: Article card #1027 #1028

merged 8 commits into from Oct 21, 2021

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Sep 29, 2021

image
image
image

API to review:

/** Create an article card for longer texts. */
interface State {
  /** The card’s title, displayed at the top. */
  title: S
  /** The card's subtitle, displayed under the title. */
  subtitle?: S
  /** The card's caption, displayed under the subtitle. */
  caption?: S
  /** Markdown text. */
  content?: S
}

Used card-wide commands prop that is auto-added to every card and hid the rendered menu in top right corner. wdyt @lo5 ?

Design notes:

  • Didn't implement image the dot separator. IIRC, @lo5 mentioned in his feedback he doesn't want it.
  • Couldn't really align title and toolbar as the design doesn't account for hover effects:
    image - what would be the best to do in this case? - cc @sandruparo @shihan007 @shanaka-rajitha

Closes #1027

@shihan007
Copy link

shihan007 commented Sep 29, 2021

@mturoci better to align title and toolbar with base line of the font. and reduce space between heading 1 and paragraph.

cc: @shanaka-rajitha @sandruparo

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 29, 2021

better to align title and toolbar with base line of the font

Even for price of breaking the card's padding?

@shihan007
Copy link

Web 1920 – 1
@mturoci like this

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 29, 2021

@shihan007 this doesn't answer my previous question. The hover effect will ignore the 24px card padding if I do it. Is it okay?

and reduce space between heading 1 and paragraph.

The spaces are based on the design - 24px top and 16px bottom margin for title. What should be the correct values then?

@shihan007
Copy link

@mturoci why do we need a toolbar for this card layout? It wasn't in the requirement doc or in the design

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 29, 2021

It was there all the time, the card just got renamed.
image

Also in the design:
image

@shihan007
Copy link

shihan007 commented Sep 29, 2021

@mturoci these are icons which does not take more that 24px x 24px space. placing buttons like that takes lot of space.

@shihan007
Copy link

@mturoci if we are placing icons lets middle align icons with the title. Hover effect not a issue

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 29, 2021

Thanks @shihan007, what about the correct spacing between title and paragraph?

@shihan007
Copy link

@mturoci for cards like these better to have a small size toolbar (height set to 24px, font size 12px). For containers and pages we can use normal toolbar (font size:14px and default spacing)

Space between title section(title+subtitle) and paragraph = 16px
within two sections we can use 24px space

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 29, 2021

@shihan007 should be good now - 24px toolbar

image

@shihan007
Copy link

@mturoci yeah this is great. Apply mentioned spacing on title and paragraph too

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 30, 2021

Already done, I updated the latest screenshots in a PR description, feel free to check.

@lo5
Copy link
Member

lo5 commented Oct 4, 2021

Agree with @shihan007 - The large toolbar doesn't fit in here. This is meant to be a card for long-form content, with only minor actions (upvote / share) associated with it, if any. The full / right-aligned toolbar seems overbearing. I'd prefer what was requested in the original design (link) - a title, no subtitle, no caption, a de-emphasized set of icon-links, and content (markdown).

@lo5
Copy link
Member

lo5 commented Oct 4, 2021

Also see comment about ui.mini_buttons(): #1033 (comment)

@mturoci
Copy link
Collaborator Author

mturoci commented Oct 6, 2021

Implemented with ui.mini_buttons:

image
image
image

API

/** Create a set of mini buttons laid out horizontally. */
export interface MiniButtons {
  /** The buttons in this set. */
  items: MiniButton[]
}

/** Create a mini button - same as regular button, but smaller in size. */
export interface MiniButton {
  /** An identifying name for this component. If the name is prefixed with a '#', the button sets the location hash to the name when clicked. */
  name: Id
  /** The text displayed on the button. */
  label: S
  /** An optional icon to display next to the button label. */
  icon?: S
}

/** Create an article card for longer texts. */
interface State {
  /** The card’s title, displayed at the top. */
  title: S
  /** Markdown text. */
  content?: S
  /** Collection of small buttons rendered on the other side of card's title. */
  mini_buttons?: MiniButtons
}

@lo5
Copy link
Member

lo5 commented Oct 12, 2021

@mturoci Looks good. Minor changes:

  • Place mini-buttons directly below title, left-aligned like in the original design.
  • Change State.mini_buttons to State.items (consistent with form_card).
  • Use H2 onwards in example content so that there is visual distinction between the card's title and the top-level headings inside the content.

@mturoci
Copy link
Collaborator Author

mturoci commented Oct 12, 2021

All comments addressed:
image

cc @lo5 for final design review

@shihan007
Copy link

shihan007 commented Oct 12, 2021

@lo5 @mturoci I think icons should align to right top to maintain consistency with other cards. And I can see subtitle also missing here. wdyat?

cc: @sandruparo @shanaka-rajitha

@mturoci mturoci merged commit 0aa05e3 into master Oct 21, 2021
@mturoci mturoci deleted the feat/issue-1027 branch October 21, 2021 11:42
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.

Article card
3 participants