feat(telekom-header): data api #1535
Conversation
✅ Deploy Preview for marvelous-moxie-a6e2fe ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
nowseemee
force-pushed
the
data-api
branch
2 times, most recently
from
6493c60 to
742d4fe
Compare
nowseemee
force-pushed
the
data-api
branch
2 times, most recently
from
b508c82 to
2f4cf55
Compare
There was a problem hiding this comment.
These is just ace. Thanks a lot @nowseemee — I'm requesting some small changes and leaving a note (regarding those classes).
| @@ -185,3 +212,59 @@ scale-telekom-nav-flyout { | |||
| </scale-telekom-mega-menu-column> | |||
| </scale-telekom-mega-menu> | |||
| ``` | |||
|
|
|||
| ## Data API | |||
There was a problem hiding this comment.
I would append "(Backward compatibility)" to this title (only here, not in the menu, if possible) so it reads:
## Data API (Backward compatibility)
| @@ -185,3 +212,59 @@ scale-telekom-nav-flyout { | |||
| </scale-telekom-mega-menu-column> | |||
| </scale-telekom-mega-menu> | |||
| ``` | |||
|
|
|||
| ## Data API | |||
|
|
|||
There was a problem hiding this comment.
I would add a paragraph here with the following (or similar) notice, please let me know what you think:
This
scale-telekom-header-data-back-compatcomponent is only meant to make it easier to migrate from a previous version of the Brand Header. If you do not need to migrate, please avoid it.
There was a problem hiding this comment.
I would even add a link to the fixtures.js file in GitHub as a form of "API reference"… 🤔
| tag: 'scale-telekom-header-data-back-compat', | ||
| shadow: false, | ||
| }) | ||
| export class TelekomHeader { |
There was a problem hiding this comment.
Wouldn't you rename the class name to TelekomHeaderDataBackCompat?
| <scale-telekom-nav-item class="burger-item"> | ||
| <button> | ||
| <scale-badge> | ||
| <scale-icon-action-menu></scale-icon-action-menu> | ||
| </scale-badge> | ||
| </button> | ||
| <scale-telekom-nav-flyout class="mobile-nav-flyout"> |
There was a problem hiding this comment.
These burger-item and mobile-nav-flyout classes are 💩 because people will need the code but it's "hidden", no? Please don't do anything about them now.
I'll try and add an update to scale-telekom-header now (probably some news props) so we can remove them (here and in the .vue file where they're defined).
There was a problem hiding this comment.
The classes are my fault in the first place… 🫣
|
Thanks for the review @acstll, I've pushed a commit. |
No description provided.