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

feat: Adding a step two in wizard form #4905

Merged
merged 19 commits into from
Sep 1, 2020
Merged

feat: Adding a step two in wizard form #4905

merged 19 commits into from
Sep 1, 2020

Conversation

maze-runnar
Copy link
Contributor

Fixes #4671

Short description of what this resolves:

Adding a new step in wizard

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Aug 28, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/rlpc2ityt
✅ Preview: https://open-event-frontend-git-fork-maze-runnar-patch-6.eventyay.vercel.app

@auto-label auto-label bot added the feature label Aug 28, 2020
@maze-runnar maze-runnar changed the title feat: Add a step two with the heading "Additional Info and Settings" feat: Adding a step two in wizard form Aug 28, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 3 alerts when merging f65366c into 3d0b2df - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@iamareebjamal
Copy link
Member

Shouldn't there be code deletion from basic details step JS?

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #4905 into development will decrease coverage by 0.30%.
The diff coverage is 2.70%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #4905      +/-   ##
===============================================
- Coverage        23.17%   22.86%   -0.31%     
===============================================
  Files              481      483       +2     
  Lines             5096     5121      +25     
  Branches            18       18              
===============================================
- Hits              1181     1171      -10     
- Misses            3911     3946      +35     
  Partials             4        4              
Impacted Files Coverage Δ
app/components/forms/wizard/basic-details-step.js 18.57% <ø> (-5.82%) ⬇️
app/components/forms/wizard/other-details-step.js 0.00% <0.00%> (ø)
app/controllers/events/view/edit/attendee.js 0.00% <ø> (ø)
app/controllers/events/view/edit/basic-details.js 0.00% <ø> (ø)
app/controllers/events/view/edit/other-details.js 0.00% <0.00%> (ø)
app/mixins/event-wizard.js 1.13% <ø> (ø)
app/router.js 100.00% <100.00%> (ø)
app/utils/computed-helpers.js 22.50% <0.00%> (-7.50%) ⬇️
app/components/widgets/forms/link-input.js 42.10% <0.00%> (-5.27%) ⬇️
app/helpers/ui-grid-number.js 34.78% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1474c3...7d45a6a. Read the comment docs.

@maze-runnar
Copy link
Contributor Author

Shouldn't there be code deletion from basic details step JS?

yes, I would do that.

}),
// licenses: computed(function() {
// return orderBy(licenses, 'name');
// }),
Copy link
Member

Choose a reason for hiding this comment

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

Please delete, don't comment

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 2 alerts when merging d84211e into 6c6d9e4 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 9 alerts when merging 322150a into 6c6d9e4 - view on LGTM.com

new alerts:

  • 8 for Unused variable, import, function or class
  • 1 for Duplicate property

@iamareebjamal
Copy link
Member

It seems that validation rules are just copied from basic details to other step. Please remove unneeded code from both steps. I wanted to merge this before including work on online event form because then it'll need to be updated again but going ahead with online form with old UI for now

@maze-runnar
Copy link
Contributor Author

It seems that validation rules are just copied from basic details to other step. Please remove unneeded code from both steps. I wanted to merge this before including work on online event form because then it'll need to be updated again but going ahead with online form with old UI for now

@iamareebjamal I have cleared the validations and included only required validations now.

Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

Complexity increasing per file
==============================
- app/components/forms/wizard/other-details-step.js  4
         

See the complete overview on Codacy

licenceUrl : license.link
});
},
saveDraft() {
Copy link
Member

Choose a reason for hiding this comment

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

prompt : this.l10n.t('Please enter a valid url')
}
]
},
Copy link
Member

Choose a reason for hiding this comment

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

Codacy found an issue: Unexpected trailing comma.

iamareebjamal
iamareebjamal previously approved these changes Sep 1, 2020
@mariobehling
Copy link
Member

Works perfectly. Thanks!

I added an issue to add this step to the dashboard as well here #4936

@mariobehling
Copy link
Member

@maze-runnar Please make a small change. I think it is too small to open an issue for that. So, I am just posting it here.

Change "Extra Details".. to "Extra details".. in the wizard top button, please.

@snitin315
Copy link
Member

@maze-runnar Please make a small change. I think it is too small to open an issue for that. So, I am just posting it here.

Change "Extra Details".. to "Extra details".. in the wizard top button, please.

Just created a follow-up PR. #4941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wizard: Simplify UX by separating step 1 into two steps
5 participants