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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add available services to meetings #3150

Merged
merged 7 commits into from
Apr 19, 2018
Merged

Conversation

rbngzlv
Copy link
Contributor

@rbngzlv rbngzlv commented Apr 8, 2018

馃帺 What? Why?

The admin can define what services will be available in the meeting (simultaneous translation, space enabled for people with functional diversity, children's area, ...)

馃搶 Related Issues

馃搵 Subtasks

  • Add specs
  • Add CHANGELOG entry

馃摲 Screenshots (optional)

screen shot 2018-04-18 at 18 41 45

@ghost ghost assigned rbngzlv Apr 8, 2018
@ghost ghost added the status: WIP label Apr 8, 2018
@xabier xabier mentioned this pull request Apr 8, 2018
16 tasks
@rbngzlv rbngzlv mentioned this pull request Apr 8, 2018
8 tasks
@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 10, 2018

@decidim/lot-core, @deivid-rodriguez I have duplicated three javascripts files from the surveys component to make the following UI in the admin.

screen shot 2018-04-10 at 16 14 23

I think this javascripts are valid for any nested resource, I'm right? If yes, what do you think about moving this files to the admin component?

@deivid-rodriguez
Copy link
Contributor

@rbngzlv They were definitely developed with reusability in mind! :) I agree with moving them to decidim-admin instead of copying the files 馃憤

@rbngzlv rbngzlv force-pushed the feature/meeting_services branch 3 times, most recently from 7f43eeb to 037065f Compare April 18, 2018 16:40
@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 18, 2018

@Crashillo can you check my css additions, please?

@rbngzlv rbngzlv force-pushed the feature/meeting_services branch 2 times, most recently from 46f16c5 to bbfb839 Compare April 18, 2018 23:08
@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 19, 2018

@decidim/lot-core ready to review!

@oriolgual oriolgual merged commit 9329b0f into master Apr 19, 2018
@oriolgual oriolgual deleted the feature/meeting_services branch April 19, 2018 07:05
@Crashillo
Copy link
Member

Sorry my late response! Well actually just 15h later! 馃槤 It is a couple of things we may enhance, I'll write them down as review comments.
Is it possible to restore the branch, reopen the issue, quick fix, merge, close the issue, remove the branch?? 馃槆

@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 19, 2018

Don't worry, I'll made a new PR with your comments. 馃槃

@@ -413,6 +413,10 @@ $datetime-bg: $primary;
display: none;
fill: $anchor-color;

&.alert{
Copy link
Member

Choose a reason for hiding this comment

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

There's a mixin called modifiers, that you might use to wrap all possibilities (even they're not in use).
Instead of this block, switch it by @include modifiers(fill);. Now, no matter which status you assign to the parent element, the property fill will change accordingly

Copy link
Member

Choose a reason for hiding this comment

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

Extra tip: I miss $anchor-color is a custom color status, so in that case.... @include modifiers(fill, (alert: $anchor-color)). Now I see again, it's not a big deal, but it's a way to handle statuses in the same way. You'll find other examples in the same file

@@ -440,6 +444,10 @@ $datetime-bg: $primary;
}
}

.card--list__text--top{
Copy link
Member

Choose a reason for hiding this comment

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

If this property-modified always comes along with .card--list__text, move it inside that block, and and a &. Like:

.card--list__text {
  ...
  // other properties

  &.card--list__item--top{
     // your properties
  }
}

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

Successfully merging this pull request may close these issues.

None yet

5 participants