Skip to content

Updated collection link and modal in channelSetList - #5257

Merged
akolson merged 4 commits into
learningequality:unstablefrom
yeshwanth235:issue-5234
Aug 14, 2025
Merged

Updated collection link and modal in channelSetList#5257
akolson merged 4 commits into
learningequality:unstablefrom
yeshwanth235:issue-5234

Conversation

@yeshwanth235

@yeshwanth235yeshwanth235 commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Summary

Updated ActionLink to KButton
Updated MessageDialog to KModal

Screenshots:
Screenshot 2025-08-06 at 10 48 49 AM
image
image
Screenshot 2025-08-06 at 10 49 10 AM

References

5234

Reviewer guidance

Test new KButton and KModal in channels/#/collections
It should be working as expected.

@MisRob

Copy link
Copy Markdown
Member

Thanks again @yeshwanth235! To this pull request, we will too assign a reviewer next week.

</div>
</template>
<template #actions>
<KButton @click="infoDialog = false"> {{ $tr('cancelButtonLabel') }} </KButton>

@akolsonakolsonAug 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is necessary, We could just use the inbuilt cancel and submit labels and events that come with the KModal. please see the documentation here. Both cancel and submit are optional

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Essentially, let's not use the actions slot

<VBtn @click="infoDialog = false">
{{ $tr('cancelButtonLabel') }}
</VBtn>
<template>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This template wrapping is probably not necessary too

<ActionLink
<KButton
:text="$tr('aboutChannelSetsLink')"
class="mx-2"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we implement our own css (with an appropriate name) so we completely clean up any vuetify related code from this change. I think mx-2 applies a margin of 8px on the left and right of the button.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hi @akolson
Need your input here.
Should I create a new class for this margin in this component? or
Should I create a new SCSS file for generic, project-wide styles?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A new class will do within the ChannelSetList.vue component should do just fine.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated the pr @akolson. Please help to review

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work @yeshwanth235! Just a few clean up notes and we should be good!

<p>
{{ $tr('channelSetsInstructionsText') }}
</p>
<p :style="{ color: $themePalette.red.v_500 }">

@akolsonakolsonAug 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yeshwanth235 can we use a class instead for this (as its better maintainable)? That is by defining a computed prop and using :class="$computedClass(channelSetsDisclamerStyle)" instead?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

updated it @akolson

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For future work, just noting that ordinary dynamic :style=channelSetsDisclamerStyle would work too ($computedClass use is needed for few special cases when :style cannot be used - e.g. for pseudo-selectors)

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @yeshwanth235! One more change and we should be good with this issue. Thanks and great work!

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks @yeshwanth235 for your contribution!

@akolson
akolson merged commit 9e06205 into learningequality:unstableAug 14, 2025
13 checks passed
Sign up for freeto 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.

3 participants

@yeshwanth235@MisRob@akolson