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

Theme updates feature branch (for 0.15.3) #8918

Merged
merged 14 commits into from
Apr 27, 2022
Merged

Conversation

indirectlylit
Copy link
Contributor

Summary

Feature branch for theme-related updates that would be safer to release in our first patch to avoid blocking 0.15.0 or introducing regressions.

References

See references below description

Reviewer guidance

After 0.15.0 is released we should begin testing this and preparing for 0.15.1.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@indirectlylit indirectlylit added this to the 0.15.1 milestone Dec 14, 2021
@marcellamaki marcellamaki modified the milestones: 0.15.1, 0.15.2 Feb 14, 2022
@marcellamaki
Copy link
Member

@indirectlylit -- thanks for preparing this, it looks great. I have reviewed the code and would like to see if we can do some manual QA and get this into an upcoming patch. Do you have a suggestion of how to do some testing? Thank you!

@indirectlylit
Copy link
Contributor Author

Hey @marcellamaki!

There should not be any user-facing changes here, so some simple sanity checks checking theming behavior should suffice. In particular, I would double check that Kolibri looks right with both the default theme and at least one of the other partner theme plugins applied.

More importantly, this PR adds two new functions, validateObject and objectWithDefaults, which bring Vue prop-like validation and default behavior to nested javascript objects. This can be used with component props, vuex and API validation, and other scenarios.

see:

This new functionality could be worth sharing with the team.

@pcenov
Copy link
Member

pcenov commented Apr 4, 2022

@radinamatic the only issue I was able to identify is that the info sidebar is now displayed to the left of the screen instead of being displayed to the right but I'm not sure if it's caused by this PR so posting it here as FYI. The issue is observed in all supported browsers.
Info sidebar:
2022-04-04_18-00-17

Folder resources sidebar:
2022-04-04_17-58-16

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Overall this makes sense - the main question for me is whether the validation is going to actually ever be used, as the theme object is rarely changed in the development environment?

@rtibbles rtibbles changed the title Theme updates feature branch (for 0.15.1) Theme updates feature branch (for 0.15.3) Apr 25, 2022
@indirectlylit
Copy link
Contributor Author

The main goal was to help engineers creating new themes by strictly defining the theme API

@rtibbles rtibbles marked this pull request as ready for review April 26, 2022 16:15
@pcenov
Copy link
Member

pcenov commented Apr 27, 2022

@rtibbles had another look at this using the following build and the issues mentioned above are fixed. No new issues observed.

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Manual QA by @pcenov was successful, good to go! 👍🏽

@rtibbles rtibbles merged commit ef30724 into release-v0.15.x Apr 27, 2022
@rtibbles rtibbles deleted the theme-updates branch April 27, 2022 20:54
@rtibbles rtibbles mentioned this pull request Jul 11, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants