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

Cosmetic metadata widget changes, persist changes on full width toggl… #4667

Merged
merged 20 commits into from
Nov 1, 2024

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Oct 18, 2024

SDESK-7341
SDESK-7417
SDESK-7340

@tomaskikutis
Copy link
Member

Could you post a screenshot with profile selection?

@thecalcc
Copy link
Contributor Author

thecalcc commented Oct 25, 2024

Could you post a screenshot with profile selection?

Yes, here it is.

Unopened:
image


Opened:
image

secondaryToolbarWidgets: Array<React.ComponentType<{item: T}>>;
secondaryToolbarWidgets: Array<React.ComponentType<{
item: T;
onChange(itemWithChanges: T): void;
Copy link
Member

Choose a reason for hiding this comment

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

Would this "onChange" actually reinitialize? If so I think we should call it "reinitialize" so it is clear that it will be performance intensive and can't be called on every key press.

@tomaskikutis
Copy link
Member

Can you revert the latest commit and prepare the branch to be merged? Let's do widget persistance in another PR

@thecalcc thecalcc enabled auto-merge (squash) October 28, 2024 12:46
@thecalcc
Copy link
Contributor Author

thecalcc commented Oct 28, 2024

Showcase of how the reworked toolbar style looks

image image image

Authoring itself becomes scrollable, toolbar included.
image

@thecalcc
Copy link
Contributor Author

thecalcc commented Oct 28, 2024

What needs to be decided is if we should have a third toolbar. In authoring angular this is how the view currently looks (and it is as a part of the heather fields):
image

@tomaskikutis
Copy link
Member

only use && for booleans or renderng JSX directly. In case you have a variable like const a = b && <div>b</div>
use a ternary const a = b == null ? null : <div>b</div>

Replace classname soup(className="flex flex-row flex-wrap align-center px-2 gap-1 py-1") with inline styles or a single classname.

Regarding the third toolbar - let's not have it. It doesn't show anything that is not already displayed elsewhere. In the long run we'll probably make it more customizable so it would be possible to set up any number of toolbars from root repo.

@thecalcc
Copy link
Contributor Author

Replace classname soup(className="flex flex-row flex-wrap align-center px-2 gap-1 py-1") with inline styles or a single classname.

What's the difference? Only style of how we write CSS? And if we change our spacing at some point, that wouldn't be reflected.

@tomaskikutis
Copy link
Member

What's the difference?

Easier to work with. Why spend time on coming up and remembering class names when we can use standard CSS rules directly. For spacing and other things - use variables. <div style={{padding: 'var(--padding-small)'}} />

@thecalcc
Copy link
Contributor Author

I've made it look the same as in Angular, after talking to fritz:
image

@@ -1491,26 +1503,43 @@ export class AuthoringReact<T extends IBaseRestApiResponse> extends React.PureCo
</ButtonGroup>
</div>
)}
headerStyles='authoring-header--padding-medium pt-1'
Copy link
Member

Choose a reason for hiding this comment

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

do not use CSS classnames coming outside of this project. Address other occurences of the same in this PR too.

@thecalcc thecalcc enabled auto-merge (squash) November 1, 2024 10:49
@thecalcc thecalcc merged commit 72d8679 into superdesk:develop Nov 1, 2024
3 checks passed
@@ -1502,7 +1502,7 @@ export class AuthoringReact<T extends IBaseRestApiResponse> extends React.PureCo
</ButtonGroup>
</div>
)}
headerStyles="authoring-header--padding-medium pt-1"
headerPadding={{top: 8}}
Copy link
Member

Choose a reason for hiding this comment

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

should have been paddingBlockStart

@tomaskikutis
Copy link
Member

Hey, I missed an important issue here. I though you were doing a profile selector via angular wrapper thing. You can't put it inside authoring-react.ts, because authoring-react is generic. You don't know if it's an article or some other entity you're dealing with. The compiler was warning you 😄

@thecalcc
Copy link
Contributor Author

Move the usage from authoring-react and pass it through the api or defaultToolbarItems in angular wrapper

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.

2 participants