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

Allow to remove the background and select a custom colour + cypress #34696

Merged
merged 4 commits into from
Nov 29, 2022

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 20, 2022

Fix #34511
Fix #34449
Fix #34560

  • Fix variables
  • List use cases
  • Add colour extract from custom background
  • Add cypress testing and drop selenium
  • Cleanup code

Later

@skjnldsv skjnldsv self-assigned this Oct 20, 2022
@skjnldsv skjnldsv added this to the Nextcloud 26 milestone Oct 20, 2022
@skjnldsv skjnldsv force-pushed the fix/custom-color branch 5 times, most recently from 020308e to 9617ad6 Compare November 24, 2022 17:51
@skjnldsv skjnldsv marked this pull request as ready for review November 24, 2022 18:17
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 24, 2022
@skjnldsv skjnldsv changed the title [WIP] Allow to remove the background and select a custom colour Allow to remove the background and select a custom colour + cypress Nov 24, 2022
@skjnldsv skjnldsv force-pushed the fix/custom-color branch 2 times, most recently from 683db53 to ebb9b85 Compare November 25, 2022 08:37
@skjnldsv skjnldsv requested review from PVince81, juliushaertl, come-nc, artonge, a team and szaimen and removed request for a team November 25, 2022 08:38
@skjnldsv
Copy link
Member Author

Let's go! Cypress tests are in and green!

I'm covering the following use cases:

  • Checking the default background and colour of a new user
  • Selecting a shipped background
  • Removing the background
  • Changing the primary color from a user
  • Selecting a custom background and checking the extracted primary color

@szaimen

This comment was marked as resolved.

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 25, 2022

  1. it's definitely a bug.I might have missed something during the rebase
  2. yes, we cannot go around that. We can't detect the top-header brightness of the image (not without an heavier mechanism)
  3. a. can be adjusted later, it's a good question. I think it's good enough tbh. We could put it first maybe?
    b. also can be adjusted as a follow-up. We can add that easily sure 👍

@skjnldsv
Copy link
Member Author

Is it intentional that the default background image is not shown on the login page?

Fixed

# Needed for some specific code workarounds
TESTING: true
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }}
Copy link
Contributor

Choose a reason for hiding this comment

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

For photos, I made the action upload the snapshots folder. It helps to debug sometime. Feel free to add something like:

      - name: Upload snapshots
        uses: actions/upload-artifact@v3
        if: always() // maybe only for failing tests. Does failing() exists ?
        with:
          name: snapshots
          path: cypress/snapshots

Copy link
Member Author

Choose a reason for hiding this comment

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

They are added on the cypress dashboard automatically

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, and I knew that at the time, can't remember why I added that then...

Copy link
Contributor

@artonge artonge Nov 28, 2022

Choose a reason for hiding this comment

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

I remember now ! I was not about snapshots, but nextcloud logs:

     - name: Extract NC logs
        if: always()
        run: docker-compose --project-directory cypress logs > nextcloud.log

      - name: Upload NC logs
        uses: actions/upload-artifact@v3
        if: always()
        with:
          name: nc_logs_${{ matrix.containers }}
          path: nextcloud.log

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point!

cypress/dockerNode.ts Show resolved Hide resolved
@szaimen
Copy link
Contributor

szaimen commented Nov 28, 2022

@skjnldsv

  1. Is it intentional that the default background image is not shown on the login page?

Fixed

Great :)

  1. yes, we cannot go around that. We can't detect the top-header brightness of the image (not without an heavier mechanism)

Okay, would be really good to fix this as it will always lead to problems. Shall I create a follow-up issue for this? But honestly it would be much better if that could be fixed directly in this PR...

  1. a. can be adjusted later, it's a good question. I think it's good enough tbh. We could put it first maybe?

Yeah, putting it first would be better than the current behaviour, however I think it should be really separate from the grid...

b. also can be adjusted as a follow-up. We can add that easily sure 👍

Yes, would be really good to have that. Would you also directly do this in this PR? :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 29, 2022

Shall I create a follow-up issue for this? But honestly it would be much better if that could be fixed directly in this PR...

It has already been discussed and ruled out :)
As said, it is not reasonably possible.

@skjnldsv
Copy link
Member Author

Yes, would be really good to have that. Would you also directly do this in this PR? :)

No, this PR is too big, and I'm not doing a rebase danse :)
Followups are easier to reviewand faster to get in

apps/theming/lib/Service/BackgroundService.php Outdated Show resolved Hide resolved
apps/theming/lib/Themes/CommonThemeTrait.php Outdated Show resolved Hide resolved
apps/theming/lib/ThemingDefaults.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 29, 2022
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@szaimen szaimen merged commit 7856820 into master Nov 29, 2022
@szaimen szaimen deleted the fix/custom-color branch November 29, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement feature: theming
Projects
None yet
4 participants