Skip to content

Conversation

@marcoambrosini
Copy link
Contributor

Needed to fit talk's description in the header slot.

Signed-off-by: Marco Ambrosini [email protected]

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should even rename the slot :p

@marcoambrosini
Copy link
Contributor Author

How would you like it named?? :)

@skjnldsv
Copy link
Contributor

How would you like it named?? :)

No clue, could be description. I am not the best to find names 🙈

@marcoambrosini marcoambrosini force-pushed the remove-sidebar-slot-max-height branch from d8460a1 to ce6ffad Compare November 11, 2020 12:34
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, since we're at it with the breaking changes, rename secondary-actions to actions ? As well as tertiary-actions to star ? 🤔 🤷

cc @raimund-schluessler

@marcoambrosini
Copy link
Contributor Author

I just wanted to remove a css rule though 😅

@raimund-schluessler
Copy link
Contributor

I just wanted to remove a css rule though 😅

Would this be a breaking change as well?

Renaming these slots requires every app using the appsidebar to adjust (I don't mind though, I don't use it yet, just thinking loud 🙈)

As well as tertiary-actions to star ?

It's only a star by default, but since it's a slot, it can be anything. So star might be to specific as a slot name.

@marcoambrosini
Copy link
Contributor Author

I'm fine with anything here, just lmk what to do :)

@skjnldsv
Copy link
Contributor

Would this be a breaking change as well?

There is another pr that touches the sidebar that is breaking change yes :)

@skjnldsv
Copy link
Contributor

It's only a star by default, but since it's a slot, it can be anything. So star might be to specific as a slot name.

I would argue that 'tertiary-actions' isn't really self-explanatory either 😛

@marcoambrosini
Copy link
Contributor Author

So should I do this?? #1566 (review)

@marcoambrosini marcoambrosini force-pushed the remove-sidebar-slot-max-height branch 2 times, most recently from c2c66c5 to da0637a Compare November 13, 2020 15:16
@skjnldsv skjnldsv added 2. developing Work in progress feature: app-sidebar Related to the app-sidebar component enhancement New feature or request and removed 3. to review Waiting for reviews labels Nov 16, 2020
@marcoambrosini marcoambrosini force-pushed the remove-sidebar-slot-max-height branch from da0637a to 7dec2b5 Compare November 19, 2020 09:16
@marcoambrosini
Copy link
Contributor Author

I got back to renaming only the slot and classes referring to that slot because of naming conflicts. Let's address the rest of the names in separate PRs if needed.

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 23, 2020
@skjnldsv skjnldsv merged commit a27e5ff into master Nov 23, 2020
@skjnldsv skjnldsv deleted the remove-sidebar-slot-max-height branch November 23, 2020 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature or request feature: app-sidebar Related to the app-sidebar component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants