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

[#3688] Add a new registration column in variables with summary #3894

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Feb 15, 2024

Part of #3688

Styling TBD

@Viicos Viicos linked an issue Feb 15, 2024 that may be closed by this pull request
33 tasks
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a8b6440) 96.34% compared to head (1c16984) 96.34%.
Report is 5 commits behind head on master.

❗ Current head 1c16984 differs from pull request most recent head ae9a587. Consider uploading reports for the commit ae9a587 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3894   +/-   ##
=======================================
  Coverage   96.34%   96.34%           
=======================================
  Files         711      711           
  Lines       22252    22252           
  Branches     2554     2554           
=======================================
  Hits        21438    21438           
  Misses        566      566           
  Partials      248      248           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Viicos Viicos force-pushed the feature/3688-registration-var-summary branch from 13127d1 to 0215eba Compare February 15, 2024 16:54
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Probably for a separate PR, but we will also need to add this mapping to the static variables tab 😬

defaultMessage: 'Edit variable registration',
description: "'Edit variable registration' icon label",
})}
extraClassname="fa-lg actions__action"
Copy link
Member

Choose a reason for hiding this comment

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

the actions__action class probably doesn't have any effect because there's no .actions parent.

Suggested change
extraClassname="fa-lg actions__action"
extraClassname="fa-lg"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to use it or it wouldn't to anything on hover

Copy link
Member

Choose a reason for hiding this comment

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

it violates the BEM principle, an element cannot exist without being contained inside its parent block.

Comment on lines +64 to +70
const formVariableConfigured = backendInfo.formVariableConfigured(variableKey, backend.options);
if (formVariableConfigured) {
// a summary handler is guaranteed to exist if `configurableFromVariables` is true:
const SummaryHandler = BACKEND_OPTIONS_FORMS[backend.key].summaryHandler;
const registrationSummary = (
<SummaryHandler variableKey={variableKey} backendOptions={backend.options} />
);
Copy link
Contributor Author

@Viicos Viicos Feb 16, 2024

Choose a reason for hiding this comment

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

I initially wanted to this formVariableConfigured logic inside the component directly:

If the variable wasn't registered, it would return null.

However, doing:

const registrationSummary =
  <SummaryHandler variableKey={variableKey} backendOptions={backend.options} />
);

registrationSummary cannot be checked against null, as it will be a JSX component no matter the return value


So I had to an extra formVariableConfigured function to explicitly check for this, not ideal :/

@Viicos Viicos force-pushed the feature/3688-registration-var-summary branch from a771cd4 to 1c16984 Compare February 16, 2024 13:44
defaultMessage: 'Edit variable registration',
description: "'Edit variable registration' icon label",
})}
extraClassname="fa-lg actions__action"
Copy link
Member

Choose a reason for hiding this comment

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

allowing this for now until the styles are sorted out, but ideally the icons would follow the rest of the UI and be to the left:

<div style={display: 'flex'}>
  <div className="actions actions--vertical">
    <FAIcon extraClassName="actions__action" />
  </div>
  <div>
    ...
  </div>
<div>

@sergei-maertens sergei-maertens merged commit 4fb39d5 into master Feb 19, 2024
18 of 20 checks passed
@sergei-maertens sergei-maertens deleted the feature/3688-registration-var-summary branch February 19, 2024 15:14
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.

Objects API registration backend - Improve JSON content field / mapping editor
2 participants