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

fix: removing ticket group, some extra line and adding a popup #5084

Merged
merged 8 commits into from
Sep 23, 2020
Merged

fix: removing ticket group, some extra line and adding a popup #5084

merged 8 commits into from
Sep 23, 2020

Conversation

maze-runnar
Copy link
Contributor

@maze-runnar maze-runnar commented Sep 18, 2020

Fixes #4623
Fixes #5095
@iamareebjamal , when i am clicking no in confirm modal, it is executing action, and on yes it's not. means that in this, yes is working as no, and no is working as yes. rest is working correctly.

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 Sep 18, 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/jk43avp17
✅ Preview: https://open-event-frontend-git-fork-maze-runnar-ticketgroup.eventyay.vercel.app

@maze-runnar
Copy link
Contributor Author

captured (2)

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #5084 into development will increase coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5084      +/-   ##
===============================================
+ Coverage        22.79%   22.88%   +0.08%     
===============================================
  Files              489      489              
  Lines             5173     5166       -7     
  Branches            35       35              
===============================================
+ Hits              1179     1182       +3     
+ Misses            3989     3979      -10     
  Partials             5        5              
Impacted Files Coverage Δ
app/components/widgets/forms/ticket-input.js 66.66% <0.00%> (-13.34%) ⬇️
app/helpers/confirm.js 11.11% <0.00%> (-5.56%) ⬇️
app/controllers/account/billing/invoices/list.js 0.00% <0.00%> (ø)
app/routes/account/billing/invoices/list.js 60.00% <0.00%> (+7.05%) ⬆️
app/components/tabbed-navigation.js 53.33% <0.00%> (+20.00%) ⬆️

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 c0b48c7...551d337. Read the comment docs.

@mariobehling
Copy link
Member

Thanks. For the pop up font, style and size please use the same styles as it is used for other pop ups elsewhere.

@mariobehling
Copy link
Member

It seems the pop up functionality is inverted. Instead of yes/no please use OK/Cancel.

  • The pop-up text states the following: "To sell hidden tickets you need to set up an access code under Event Dashboard > Tickets Tab > Access Code. [OK] [Cancel]"
    • If the user chooses "OK" add the tick in the tick box
    • If the user chooses "Cancel" do not add a tick in the box

@maze-runnar
Copy link
Contributor Author

Instead of yes/no please use OK/Cancel.

@iamareebjamal how to change button text in confirm helper?

@iamareebjamal
Copy link
Member

Make them props

@maze-runnar
Copy link
Contributor Author

how to use them in helper

@iamareebjamal
Copy link
Member

Currently they'll be hardcoded. You have to make them props and pass them in the component

@iamareebjamal
Copy link
Member

See other dialogs which are using OK/Cancel

@maze-runnar maze-runnar changed the title WIP: removing ticket group, some extra line and adding a popup fix: removing ticket group, some extra line and adding a popup Sep 18, 2020
@auto-label auto-label bot added the fix label Sep 18, 2020
@maze-runnar
Copy link
Contributor Author

Screenshot from 2020-09-18 16-40-35

@mariobehling
Copy link
Member

For the pop up font, style and size please use the same styles as it is used for other pop ups elsewhere.

@mariobehling
Copy link
Member

mariobehling commented Sep 18, 2020

  • Good that you took out the horizontal lines, but we also need to keep some spacing in the area where the horizontal line breaks used to be. So, yes get rid of the horizontal line breaks, but keep a bit of spacing in the area, please.
  • One of the PRs recently introduced a line break below "Yes, accept payment through Stripe. Please also get rid of that line break and make it look the same as the above area of Paypal.
    Screenshot from 2020-09-18 20-41-08

@maze-runnar
Copy link
Contributor Author

Screenshot from 2020-09-20 17-12-09

@maze-runnar
Copy link
Contributor Author

Screenshot from 2020-09-20 18-58-24

Copy link
Member

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

Complexity increasing per file
==============================
- app/helpers/confirm.js  1
         

See the complete overview on Codacy

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Looks good for me UI wise. @iamareebjamal Please check code. Thank you.

.then(() => {
params[1]();
});
} else if (params.length >= 2) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain please what is happening here. Looks very confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.confirm.prompt('Are You Sure?', { 'denyText': params[2], 'approveText': params[3], 'denyColor': params[4], 'approveColor': params[5], 'extra': params[0] })

the options are from service/confirm.js. In confirm-moda.js these are props (denyText, approveText...). in confirm modal helper i am using those and in component passing them.

@iamareebjamal iamareebjamal merged commit 53312b9 into fossasia:development Sep 23, 2020
@maze-runnar maze-runnar deleted the ticketgroup branch September 23, 2020 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants