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

Add CSS for CreateGroupDialog to give group ID input suffix and prefix style #5505

Merged
merged 2 commits into from Nov 3, 2017

Conversation

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Nov 3, 2017

2017-11-03-154127_652x231_scrot

@@ -33,3 +33,30 @@ limitations under the License.
background-color: $primary-bg-color;
}

.mx_CreateGroupDialog_input.has_prefix.has_suffix {

This comment has been minimized.

Copy link
@ara4n

ara4n Nov 3, 2017

Member

we have to namespace CSS classes correctly, otherwise this will collide with any other random CSS dependency (e.g. markdown, syntax highlighter, etc) which might choose to define a generic class like has_prefix. In this instance, I assume it should be something like mx_CreateGroupDialog_input_hasPrefix.

....however, it's very very unclear and confusing as to what the difference is between mx_CreateGroupDialog_input_prefix and mx_CreateGroupDialog_input.has_prefix here, and so i suspect you need a better name anyway.

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Nov 3, 2017

Author Contributor

ah, I see what you're saying

let's go with
.mx_CreateGroupDialog_prefix and .mx_CreateGroupDialog_input_hasPrefixAndSuffix

This comment has been minimized.

Copy link
@ara4n

ara4n Nov 3, 2017

Member

also, i would avoid the .foo.bar.baz selector format - we don't use it in many other places, and instead do the (different) .foo .bar, .foo .baz selector to say "select classes with name bar or baz which are within a parent class called foo". (Unless there's a performance improvement I've forgotten about explicitly having .foo.bar.baz being "select classes which have names foo, bar & baz"? I may also be a bit historically traumatised by .foo.bar.baz not working reliably on IE4 ;P)

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Nov 3, 2017

lgtm

@lukebarnard1 lukebarnard1 merged commit 5d9a1b4 into develop Nov 3, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.