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

CARDS-2513: Status flags for subjects #1754

Merged
merged 4 commits into from
Aug 1, 2024
Merged

CARDS-2513: Status flags for subjects #1754

merged 4 commits into from
Aug 1, 2024

Conversation

acrowthe
Copy link
Contributor

@acrowthe acrowthe commented Apr 12, 2024

Add status flags to subjects which are displayed in the subject chart and subject header

image

@acrowthe acrowthe changed the title Cards 2513 CARDS-2513: Status flags for subjects Apr 12, 2024
@@ -564,6 +567,10 @@ const questionnaireStyle = theme => ({
marginBottom: theme.spacing(1),
marginRight: theme.spacing(1),
},
subjectChartChip: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we reuse the subjectChip className from line 566? The two have similar names, purposes and styles, we should merge them.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is not nice. All these classes shouldn't be "something Chip", since that is a technical detail about the implementation, they should have meaningful semantic names. Maybe replace Chip with StatusFlag or even Flag. And differentiate between classes that say what kind of flag it is, and classes that say where it appears, e.g. INCOMPLETEFlag and flagInSubjectChart. But is the second case actually needed? I don't know how styles and style classes work here, but in pure CSS you could just say .subjectChart .DefaultFlag.

Just nitpicking, this isn't related to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

And to answer the question, I don't think it can be reused, one is about chips about the subject that you're looking at itself, one is about chips about other related items inside the subject chart.

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've updated the names to be a little clearer what they are for. Can't easily reuse subjectChip as they the chips don't look vertically centered with the same bottom margin

@sashaandjic
Copy link
Contributor

Would this look any better?
image

@marta-
Copy link
Contributor

marta- commented Apr 19, 2024

Would this look any better?

It's complicated... the forms under each subject are displayed in a table, so having kind of layout will be a tad difficult in most cases

@acrowthe
Copy link
Contributor Author

Layout has been updated (see the screenshot in the first comment for an example).
No easy way to get the status flags lined up due to them using different layout methods (table vs. grid) but they are close

Remove extra spaces in style sheet
Adjust css class names and improve html layout
Fix some some status flags still using chip naming scheme
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.

4 participants