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

Desktop: Add classnames to DOM elements for theming purposes #4933

Merged
merged 10 commits into from
May 17, 2021
Merged

Desktop: Add classnames to DOM elements for theming purposes #4933

merged 10 commits into from
May 17, 2021

Conversation

andrejilderda
Copy link
Contributor

This PR adds a few classnames to DOM-elements that can not be targeted with CSS in other ways. This includes most of the list that is specified here:
Joplin forum: List of CSS elements which should have a class

Including:

  • Preferences -> Encryption -> E2EE text
  • Search bar
  • Local search
  • Selected Notebooks / tags
  • Selected note in notelist
  • Note title
  • Last edit timestamp
  • Button for Markup/WYSIWYG editor
  • Search field, new todo and new note button
  • List items (notebook) and note count
  • DIV for tag area
  • Command palette/Go to anything
  • Modal dialogs

Copy link
Owner

@laurent22 laurent22 left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I made a few suggestions for the class names to make them a bit more specific, but let me know if some of them are now too specific. I guess we need to strike the right balance between being able to select a specific element, and being able to select all elements of a particular type.

The framework you've suggested before would certainly solve this but I think would require too many changes in too many places.

@@ -91,7 +91,7 @@ export default function NoteTitleBar(props: Props) {
}, []);

function renderTitleBarDate() {
return <span style={styles.titleDate}>{time.formatMsToLocal(props.noteUserUpdatedTime)}</span>;
return <span className="user-updated-time" style={styles.titleDate}>{time.formatMsToLocal(props.noteUserUpdatedTime)}</span>;
Copy link
Owner

Choose a reason for hiding this comment

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

To name the classes, could you follow the pattern "something-type", for example as you've named "tag-bar", or "title-input". So here it could be updated-time-label for example.

@@ -54,12 +54,14 @@ export default function NoteListControls(props: Props) {
return (
<ButtonContainer>
<StyledButton
className="new-todo"
Copy link
Owner

Choose a reason for hiding this comment

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

new-todo-button

tooltip={CommandService.instance().label('newTodo')}
iconName="far fa-check-square"
level={ButtonLevel.Primary}
onClick={onNewTodoButtonClick}
/>
<StyledButton
className="new-note"
Copy link
Owner

Choose a reason for hiding this comment

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

new-note-button

'list-item-container',
props.isSelected && 'selected',
item.todo_completed && 'todo-completed',
item.is_todo ? 'todo' : 'note',
Copy link
Owner

Choose a reason for hiding this comment

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

todo-list-item, note-list-item

props.isSelected && 'selected',
item.todo_completed && 'todo-completed',
item.is_todo ? 'todo' : 'note',
(props.index + 1) % 2 === 0 ? 'even' : 'odd',
Copy link
Owner

Choose a reason for hiding this comment

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

generic terms like "even" or "odd" on their own are fine though. I guess it's just when naming a component that we should use the "something-type" format.

Comment on lines 224 to 228
inputComp = <Datetime value={this.state.answer} inputProps={{ style: styles.input }} dateFormat={time.dateFormat()} timeFormat={time.timeFormat()} onChange={momentObject => onDateTimeChange(momentObject)} />;
inputComp = <Datetime className="select-datetime" value={this.state.answer} inputProps={{ style: styles.input }} dateFormat={time.dateFormat()} timeFormat={time.timeFormat()} onChange={momentObject => onDateTimeChange(momentObject)} />;
} else if (this.props.inputType === 'tags') {
inputComp = <CreatableSelect styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} value={this.state.answer} placeholder="" components={makeAnimated()} isMulti={true} isClearable={false} backspaceRemovesValue={true} options={this.props.autocomplete} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
inputComp = <CreatableSelect className="select-tag" styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} value={this.state.answer} placeholder="" components={makeAnimated()} isMulti={true} isClearable={false} backspaceRemovesValue={true} options={this.props.autocomplete} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
} else if (this.props.inputType === 'dropdown') {
inputComp = <Select styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} components={makeAnimated()} value={this.props.answer} defaultValue={this.props.defaultValue} isClearable={false} options={this.props.autocomplete} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
inputComp = <Select className="select-dropdown" styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} components={makeAnimated()} value={this.props.answer} defaultValue={this.props.defaultValue} isClearable={false} options={this.props.autocomplete} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
Copy link
Owner

Choose a reason for hiding this comment

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

datetime-picker, tag-selector, item-selector (not sure about the last one)

@@ -254,8 +254,8 @@ class PromptDialog extends React.Component {
}

return (
<div style={styles.modalLayer}>
<div style={styles.promptDialog}>
<div className="modal" style={styles.modalLayer}>
Copy link
Owner

Choose a reason for hiding this comment

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

modal-layer

@@ -72,10 +72,10 @@ function ExpandLink(props: any) {
function FolderItem(props: any) {
const { hasChildren, isExpanded, depth, selected, folderId, folderTitle, anchorRef, noteCount, onFolderDragStart_, onFolderDragOver_, onFolderDrop_, itemContextMenu, folderItem_click, onFolderToggleClick_ } = props;

const noteCountComp = noteCount ? <StyledNoteCount>{noteCount}</StyledNoteCount> : null;
const noteCountComp = noteCount ? <StyledNoteCount className="note-count">{noteCount}</StyledNoteCount> : null;
Copy link
Owner

Choose a reason for hiding this comment

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

note-count-label

@@ -359,7 +360,7 @@ class SidebarComponent extends React.Component<Props, State> {
}

renderNoteCount(count: number) {
return count ? <StyledNoteCount>{count}</StyledNoteCount> : null;
return count ? <StyledNoteCount className="note-count">{count}</StyledNoteCount> : null;
Copy link
Owner

Choose a reason for hiding this comment

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

note-count-label


return (
<div onClick={this.modalLayer_onClick} style={theme.dialogModalLayer}>
<div style={style.dialogBox}>
<div className="modal" onClick={this.modalLayer_onClick} style={theme.dialogModalLayer}>
Copy link
Owner

Choose a reason for hiding this comment

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

modal-layer

@tessus tessus added the desktop All desktop platforms label May 14, 2021
@andrejilderda
Copy link
Contributor Author

Thanks for looking into this @laurent22 ! I've changed all classnames to the ones you proposed. Good suggestions!

@laurent22
Copy link
Owner

All good now, thanks for the pull request @ajilderda!

@laurent22 laurent22 merged commit f3e03d4 into laurent22:dev May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants