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

Implemented: additional job actions, to job config component (#364tt29) #212

Merged
merged 5 commits into from
Aug 26, 2022

Conversation

rathoreprashant
Copy link
Contributor

@rathoreprashant rathoreprashant commented Aug 3, 2022

Related Issues

Closes #

Short Description and Why It's Useful

Screenshots of Visual Changes before/after (If There Are Any)

addititionalAction

runnow

additionalActionWorking.mp4

IMPORTANT NOTICE - Remember to add changelog entry

Contribution and Currently Important Rules Acceptance

@ymaheshwari1 ymaheshwari1 requested a review from azkyakhan August 3, 2022 06:34
src/components/JobConfiguration.vue Outdated Show resolved Hide resolved
src/components/JobConfiguration.vue Outdated Show resolved Hide resolved
src/components/JobConfiguration.vue Outdated Show resolved Hide resolved
@@ -70,13 +70,34 @@
<ion-button expand="block" @click="saveChanges()">{{ $t("Save changes") }}</ion-button>
</div>
</section>
<div class="job-actions">
<ion-item slot="start" @click="viewJobHistory(currentJob)" button>
Copy link
Contributor

Choose a reason for hiding this comment

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

@azkyakhan Thoughts on slot here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This slots over here are not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

<ion-item slot="end" @click="updatePinnedJobs(currentJob?.systemJobEnumId)" button>
<ion-icon slot="start" :icon="pinOutline" />
{{ $t("Pin job") }}
<ion-checkbox :checked="getPinnedJobs.includes(currentJob.systemJobEnumId)" slot="end" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I am seeing slot in different places. Why not at the same place everytime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated sir

flex-basis: 50%;
}
.job-actions > ion-item > ion-checkbox{
margin: unset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this? @azkyakhan

Copy link
Contributor

@azkyakhan azkyakhan Aug 5, 2022

Choose a reason for hiding this comment

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

Due to the margin on checkbox this item is looking like this :

Screenshot from 2022-08-05 16-44-24

That is why we have unset the margin on checkbox.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see problem here with checkbox spacing. Am I missing something?

Copy link
Contributor

@azkyakhan azkyakhan Aug 5, 2022

Choose a reason for hiding this comment

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

That item line is slight down from the adjacent item.
Untitled design

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, what if we instead align content/items to end. I'd like to avoid changing item height from native behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented align-items to end

@@ -328,7 +416,19 @@ ion-list {

.mobile-only {
display: none;
}
}
.job-actions {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have an "actions" class. So job actions doesn't make sense here I think. "More actions" seems better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated sir

Implemented 'align-items: end' property instead of 'margin: unset'
@adityasharma7 adityasharma7 merged commit 497af0b into hotwax:main Aug 26, 2022
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