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: Move and simplify "Event Types" #5651

Merged
merged 6 commits into from
Nov 17, 2020
Merged

fix: Move and simplify "Event Types" #5651

merged 6 commits into from
Nov 17, 2020

Conversation

divyamtayal
Copy link
Member

@divyamtayal divyamtayal commented Nov 15, 2020

Fixes #5014

Short description of what this resolves:

Moved up Event Types

Changes proposed in this pull request:

  • Moved up event types.
  • Removed Label
  • Working in mobile view as well

ScreenShots

Screenshot from 2020-11-16 02-33-38

(Mobile View)

Screenshot from 2020-11-16 02-34-13

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 Nov 15, 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/863dqlcre
✅ Preview: https://open-event-frontend-git-move-event-types-5014.eventyay.vercel.app

@divyamtayal
Copy link
Member Author

@iamareebjamal Please see the changes I have made.

@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #5651 (97431a3) into development (0c0c8cf) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5651      +/-   ##
===============================================
- Coverage        23.79%   23.73%   -0.06%     
===============================================
  Files              498      498              
  Lines             5275     5275              
  Branches            47       47              
===============================================
- Hits              1255     1252       -3     
- Misses            4012     4015       +3     
  Partials             8        8              
Impacted Files Coverage Δ
app/components/tabbed-navigation.js 33.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 6337818...c202f47. Read the comment docs.

@divyamtayal divyamtayal changed the title Moved Event Types up Move and simplify "Event Types" Nov 15, 2020
@iamareebjamal
Copy link
Member

Please don't take up issues labeled 'project', they're for the freelancer

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 16, 2020

@iamareebjamal next time I will take care of that, I was not knowing. but for nowI have already worked on it, can u review and if ok merge it.

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.

Thanks!

Please also do the following in this PR:

  • Adjust the box design to match the other boxes in the areas (Compare to the Timezone box)
    • Make the box height the same as the other one-line boxes
    • Make the spacing of the text the same as in the other boxes (in particular the spacing on the left side)

@mariobehling
Copy link
Member

Please also make the box line the same as the other boxes. Basically you could just reuse the design of the timezone box entirely. Thanks.

@iamareebjamal
Copy link
Member

@mariobehling That's a different component from semantic UI component because semantic UI component does not work with items with & in them. We'll have issues like #4546 if we revert to semantic UI dropdown

@divyamtayal
Copy link
Member Author

@mariobehling That's a different component from semantic UI component because semantic UI component does not work with items with & in them. We'll have issues like #4546 if we revert to semantic UI dropdown

@iamareebjamal should i make any changes now bcz this is entirely different component than UiDropdown. So pls let me know?

@divyamtayal
Copy link
Member Author

@iamareebjamal @mariobehling Please see the changes made.

Screen Shot 2020-11-17 at 13 37 28

Screenshot from 2020-11-17 13-37-03

@iamareebjamal
Copy link
Member

iamareebjamal commented Nov 17, 2020

Before:
99364033-7b820700-28db-11eb-8b12-642bc8574568 (1)
After:
99364051-8472d880-28db-11eb-94d0-6bbe33f7ed27 (1)

Options selector is much more squished now than before. And considering the height has been raised, looks much worse

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 17, 2020

Screenshot from 2020-11-17 14-35-39

Screen Shot 2020-11-17 at 14 37 37

@iamareebjamal PLease see the changes

@iamareebjamal
Copy link
Member

iamareebjamal commented Nov 17, 2020

When you are editing it anyway, why not make it the same size as the dropdown height? And change the color to grey like timezone dropdown. It still doesn't feel right that the field itself is twice as high as the options

@iamareebjamal
Copy link
Member

$ember-power-select-highlighted-background for color and you already know how to change height

@divyamtayal
Copy link
Member Author

@iamareebjamal Please see the changes
Screenshot from 2020-11-17 15-14-16

@iamareebjamal
Copy link
Member

Perfect, thanks

@iamareebjamal iamareebjamal changed the title Move and simplify "Event Types" fix: Move and simplify "Event Types" Nov 17, 2020
@auto-label auto-label bot added the fix label Nov 17, 2020
@iamareebjamal iamareebjamal merged commit 8561da4 into fossasia:development Nov 17, 2020
@divyamtayal divyamtayal deleted the Move-event-types-5014 branch November 23, 2020 00:45
@divyamtayal divyamtayal restored the Move-event-types-5014 branch November 23, 2020 00:45
@divyamtayal divyamtayal deleted the Move-event-types-5014 branch November 27, 2020 20:42
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 Step 1: Move and simplify "Event Types"
3 participants