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

Config traits #5918

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Config traits #5918

wants to merge 23 commits into from

Conversation

zoran995
Copy link
Contributor

@zoran995 zoran995 commented Oct 17, 2021

What this PR does

Fixes #
TerriaMap PR: TerriaJS/TerriaMap#557

Update TerriaJS config parameters to use traits system. Some of the parameters are renamed

  • showWelcomeMessage and welcomeMessageVideo are now parameters on object welcomeMessage as show and video
  • helpContent is now helpItems
  • helpContentTerms are now helpTerms (they are not strictly related to helpContent)
  • feedbackPreamble and feedbackMinLength are now parameters on object feedback as preamble and minLength
  • brandBarElements, brandBarSmallElements, displayOneBrand are now parameters on object brandBar as elements, smallElements, displayOneBrand
  • googleUrlShortenerKey is deprecated long time ago but still left as a config parameter
  • disclaimer is not used anywhere so I added it to deprecated list
  • globalDisclaimer is also not used anywhere - not sure if IT should be marked as deprecated as it is set from TerriaMap?

TODO

  • Check if some conditions depend only on the existence of specific configParameter in case when it is not primitiveTrait, objectTrait will always be defined and that condition should be updated
  • Convert deprecated options to the new ones - edit: added basic conversion, as I don't see that we really need anything on model level for now
  • Documentation should generate automatically (as for catalog items) - Best to do this as a separate PR
  • Introduce required option for trait system (at least for configuration if not doable for generic) - Probably best to do this as a separate PR (maybe target this one at first if this is not merged before)

p.s. this was much easier than initially expected

Checklist

  • [ ] There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist) - All existing test pass
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.

lib/Core/TerriaError.ts Outdated Show resolved Hide resolved
Comment on lines +317 to +318
//@ts-ignore
helpItems
Copy link
Contributor Author

@zoran995 zoran995 Oct 17, 2021

Choose a reason for hiding this comment

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

was getting type error

@zoran995
Copy link
Contributor Author

This is ready for review, added a basic conversion so no breaking changes are introduced in patch or minor version.

@zoran995
Copy link
Contributor Author

did another pass over this, also tried loading prod map-configs from https://github.com/TerriaJS/saas-catalogs-public, and no issue is raised to the user so I am pretty much confident that this can go without having to update the configs right away

@zoran995 zoran995 requested a review from nf-s April 9, 2022 17:50
@nf-s nf-s added the v9 label May 17, 2022
@nf-s nf-s mentioned this pull request May 31, 2022
5 tasks
type: HelpItemTraits,
name: "Help content items",
description: "The content to be displayed in the help panel.",
idProperty: "index"
Copy link
Member

@tephenavies tephenavies Jun 10, 2022

Choose a reason for hiding this comment

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

Just dropping a comment here for a future full review: I think this should be:

Suggested change
idProperty: "index"
idProperty: "itemName",

though I haven't thought through the consequences of either idProperty choices.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think itemName is good - then makes it easy to remove specific default help items

steps?: StepItemTraits[];
}

export class HelpItemTraits extends ModelTraits {
Copy link
Member

Choose a reason for hiding this comment

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

Relating to https://github.com/TerriaJS/terriajs/pull/5918/files#r894124712. If using itemName as an id you could add:

static isRemoval(helpItem: HelpItemTraits) {
  // This condition looks sensible
  return helpItem.markdownText === null && helpItem.trainerItems === null;
}

And then you could have default helpItems in terriajs that get overriden/removed in TerriaMap config using the strata system, I think. I have to play around with this branch sometime. And default help items in terriajs means translation support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants