Skip to content

Add button color variables #25774

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.

Sign up for GitHub

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

Jump to bottom
Merged
Pytal merged 2 commits into from
Oct 7, 2021
Merged

Add button color variables #25774

Pytal merged 2 commits into from
Oct 7, 2021

Conversation

@marcoambrosini
Copy link
Member

@marcoambrosini marcoambrosini commented Feb 24, 2021

No description provided.

@marcoambrosini marcoambrosini added the 2. developing Work in progress label Feb 24, 2021
@rullzer
Copy link
Member

rullzer commented Mar 22, 2021

So... what is the status here?

@marcoambrosini
Copy link
Member Author

marcoambrosini commented Mar 23, 2021

So... what is the status here?

Back on it these days :)

@marcoambrosini marcoambrosini changed the title Add ripple color variables Add button color variables Aug 3, 2021
@marcoambrosini marcoambrosini force-pushed the add-ripple-color-variables branch 2 times, most recently from e491584 to 4a4cb54 Compare August 4, 2021 08:00
@nextcloud nextcloud deleted a comment from faily-bot bot Aug 4, 2021
juliusknorr
juliusknorr reviewed Aug 5, 2021
View reviewed changes
@juliusknorr juliusknorr mentioned this pull request Aug 5, 2021
Merged
@marcoambrosini marcoambrosini force-pushed the add-ripple-color-variables branch from 4a4cb54 to 6bc07bf Compare August 6, 2021 09:31
@marcoambrosini marcoambrosini mentioned this pull request Aug 16, 2021
Merged
@szaimen szaimen added this to the Nextcloud 23 milestone Aug 23, 2021
@marcoambrosini marcoambrosini force-pushed the add-ripple-color-variables branch from 6bc07bf to 2cde710 Compare August 24, 2021 14:35
@juliusknorr juliusknorr force-pushed the add-ripple-color-variables branch from 84646bc to 5745bb3 Compare August 25, 2021 06:59
@marcoambrosini marcoambrosini marked this pull request as ready for review August 30, 2021 16:47
@marcoambrosini marcoambrosini added 3. to review Waiting for reviews enhancement design Design, UI, UX, etc. and removed 2. developing Work in progress labels Aug 30, 2021
@marcoambrosini marcoambrosini force-pushed the add-ripple-color-variables branch from 5745bb3 to f6e62b1 Compare September 30, 2021 09:18
@juliusknorr juliusknorr requested review from a team, PVince81, artonge and szaimen and removed request for a team October 5, 2021 11:02
artonge
artonge reviewed Oct 5, 2021
View reviewed changes
core/css/inputs.scss
/* Simple selector to allow easy overriding */
select,
button,
button:not(.button-vue),
Copy link
Contributor

@artonge artonge Oct 5, 2021

Choose a reason for hiding this comment

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

Is this to prevent the vue button from being styled from here ? Is this really necessary as it is probably scoped ?

Copy link
Member Author

@marcoambrosini marcoambrosini Oct 6, 2021

Choose a reason for hiding this comment

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

The server style is not scoped and it targets everything unless we do this

Copy link
Contributor

@artonge artonge Oct 6, 2021

Choose a reason for hiding this comment

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

Hmm, ok, I thought that vue components could be isolated from external CSS. Maybe I am wrong :)

Copy link
Member

@ChristophWurst ChristophWurst Oct 19, 2021

Choose a reason for hiding this comment

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

@marcoambrosini please document this breaking change in #27846. Mail and likely other apps using nc/vue break because of this change if I understand this correctly. cc @GretaD

@Pytal Pytal merged commit 005ed98 into master Oct 7, 2021
@Pytal Pytal deleted the add-ripple-color-variables branch October 7, 2021 00:33
@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 7, 2021
@raimund-schluessler
Copy link
Member

raimund-schluessler commented Oct 7, 2021

Will this be backported?

@marcoambrosini
Copy link
Member Author

marcoambrosini commented Oct 7, 2021

/backport to stable22

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 7, 2021
Merged
@raimund-schluessler
Copy link
Member

raimund-schluessler commented Oct 12, 2021

Can we backport it to stable21 and stable20 as well? Would help using the button component.

I will give backporting a try, let's see if it works.

@raimund-schluessler
Copy link
Member

raimund-schluessler commented Oct 12, 2021

/backport to stable21

@raimund-schluessler
Copy link
Member

raimund-schluessler commented Oct 12, 2021

/backport to stable20

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 12, 2021
Closed
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 12, 2021
Merged
@ChristophWurst
Copy link
Member

ChristophWurst commented Oct 12, 2021 •
edited
Loading

Can we backport it to stable21 and stable20 as well? Would help using the button component.

I see where this is coming from but I'm really not sure if backporting is such a great idea. After all any apps installed in existing Nextcloud installations of v20, v21 and v22 will work or break depending on which patch version they are on.

I would rather make a the button a breaking change in nc/vue and a hard requirement on Nextcloud 23. Then we have som certainty that the app we ship also works in the intended way.

General rule: we don't backport features. Only fixes are backported.

@raimund-schluessler
Copy link
Member

raimund-schluessler commented Oct 12, 2021 •
edited
Loading

General rule: we don't backport features. Only fixes are backported.

In that case, e.g the Tasks app can only start using the Button component once v23 is the oldest supported server version (July 2022), since we aim at supporting all still supported server versions with the same release.

@GretaD GretaD mentioned this pull request Oct 19, 2021
Merged
@marcoambrosini marcoambrosini mentioned this pull request Oct 19, 2021
Merged
@ChristophWurst ChristophWurst mentioned this pull request Jun 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst left review comments

@juliusknorr juliusknorr juliusknorr approved these changes

@artonge artonge artonge approved these changes

@Pytal Pytal Pytal approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@szaimen szaimen Awaiting requested review from szaimen

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement

Projects

None yet

Milestone

Nextcloud 23

Development

Successfully merging this pull request may close these issues.

9 participants

@marcoambrosini @rullzer @raimund-schluessler @ChristophWurst @juliusknorr @artonge @Pytal @szaimen