feat(link): add link component #972
Conversation
…b.com/Trendyol/baklava into feature/905-dropdown-icon-integration
add link component no breaking change, new component, 906
Signed-off-by: Erbil <[email protected]>
erbilnas
requested review from
dilandoogan,
Enes5519,
fatihhayri,
AykutSarac and
buseselvi
| export { default as BlTableCell } from "./components/table/table-cell/bl-table-cell"; | ||
| export { default as BlSplitButton } from "./components/split-button/bl-split-button"; | ||
| export { default as BlCalendar } from "./components/calendar/bl-calendar"; | ||
| export { default as BlDatePicker } from "./components/datepicker/bl-datepicker"; |
There was a problem hiding this comment.
why did we change casing? that could be a breaking change
There was a problem hiding this comment.
It's breaking tests and btw it already named like that
export default class BlDatepicker extends DatepickerCalendarMixin {
There was a problem hiding this comment.
We should talk with @dilandoogan
src/components/link/bl-link.ts
Outdated
| * Target URL for the link | ||
| */ | ||
| @property({ type: String, reflect: true }) | ||
| target = ""; |
There was a problem hiding this comment.
add type safety, we can inherit from anchor's types
There was a problem hiding this comment.
don't add them by hand as much as possible, there are examples of inheriting existing types from elements
There was a problem hiding this comment.
The target here is not the anchor type but the href. I want to give it a more meaningful name
src/components/link/bl-link.ts
Outdated
| * Whether the link is external | ||
| */ | ||
| @property({ type: Boolean, reflect: true }) | ||
| external = false; |
There was a problem hiding this comment.
do we need an external attribute here? adding an icon feels outside the boundaries of our design system, more like an UI kit addition
There was a problem hiding this comment.
It's nice to see an icon for external sites to show people they will leave I think, let's discuss
src/components/link/bl-link.ts
Outdated
| * Whether the link is disabled | ||
| */ | ||
| @property({ type: Boolean, reflect: true }) | ||
| disabled = false; |
There was a problem hiding this comment.
links shouldn't have a disabled state, they are not buttons
There was a problem hiding this comment.
We should discuss
src/components/link/bl-link.ts
Outdated
| @click="${this.handleClick}" | ||
| @keydown="${this.handleKeyDown}" |
There was a problem hiding this comment.
do we need to handle them specifically, doesn't browser do it for us natively?
There was a problem hiding this comment.
add other anchor element attributes
There was a problem hiding this comment.
I structured it this way in case we wanted to add event handling in the future, but you’re right we can remove it for now
There was a problem hiding this comment.
are you possibly referring to the conversation above?
here I meant adding the other required attributes such as rel etc.
Closes #906