[GH-66] Improve date format selection UI. Fixes #66 #86
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,8 +17,9 @@ export default class MeetingSettingsModal extends React.PureComponent { | |||||
| super(props); | ||||||
|
|
||||||
| this.state = { | ||||||
| hashtagPrefix: 'Prefix', | ||||||
|
There was a problem hiding this comment. Do we have a way for the user to define their own date format that is used, like the feature currently works? There was a problem hiding this comment. no, as per issue. IMO it is end goal to limit that. |
||||||
| weekdays: [1], | ||||||
| dateFormat: '1-2', | ||||||
|
There was a problem hiding this comment. Great catch, seems like I missed this while separating commits, will fix this. |
||||||
| }; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -28,20 +29,30 @@ export default class MeetingSettingsModal extends React.PureComponent { | |||||
| } | ||||||
|
|
||||||
| if (this.props.meeting && this.props.meeting !== prevProps.meeting) { | ||||||
| const splitResult = this.props.meeting.hashtagFormat.split('{{');// we know, date Format is preceded by {{ | ||||||
| const hashtagPrefix = splitResult[0]; | ||||||
| const dateFormat = splitResult[1].substring(0, splitResult[1].length - 2); // remove trailing }} | ||||||
|
There was a problem hiding this comment. Can we trust that Also if it's an existing agenda setting that is made before this PR, do we know that There was a problem hiding this comment. For the new ones, yes. Since we are assigning value from controlled values. |
||||||
| // eslint-disable-next-line react/no-did-update-set-state | ||||||
| this.setState({ | ||||||
| hashtagPrefix, | ||||||
| dateFormat, | ||||||
| weekdays: this.props.meeting.schedule || [], | ||||||
| }); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| handleHashtagChange = (e) => { | ||||||
| this.setState({ | ||||||
| hashtagPrefix: e.target.value, | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| handleDateFormat = (event) => { | ||||||
| this.setState({ | ||||||
| dateFormat: event.target.value, | ||||||
| }); | ||||||
| }; | ||||||
|
There was a problem hiding this comment. Can we be consistent and use There was a problem hiding this comment. sure, will do that. |
||||||
|
|
||||||
| handleCheckboxChanged = (e) => { | ||||||
| const changeday = Number(e.target.value); | ||||||
| let changedWeekdays = Object.assign([], this.state.weekdays); | ||||||
|
|
@@ -62,7 +73,7 @@ export default class MeetingSettingsModal extends React.PureComponent { | |||||
| onSave = () => { | ||||||
| this.props.saveMeetingSettings({ | ||||||
| channelId: this.props.channelId, | ||||||
| hashtagFormat: `${this.state.hashtagPrefix}{{${this.state.dateFormat}}}`, | ||||||
| schedule: this.state.weekdays.sort(), | ||||||
| }); | ||||||
|
|
||||||
|
|
@@ -118,19 +129,50 @@ export default class MeetingSettingsModal extends React.PureComponent { | |||||
| </div> | ||||||
| </div> | ||||||
| <div className='form-group'> | ||||||
| <div style={{display: 'flex'}}> | ||||||
|
There was a problem hiding this comment. What does setting this value do in this case? It doesn't look like there are any There was a problem hiding this comment. Actually, this expands items to fill available free space or shrinks them to prevent overflow. |
||||||
| <div | ||||||
| className='fifty' | ||||||
| style={{padding: '5px'}} | ||||||
| > | ||||||
| <label className='control-label'>{'Hashtag Prefix'}</label> | ||||||
| <input | ||||||
| onChange={this.handleHashtagChange} | ||||||
| className='form-control' | ||||||
| value={this.state.hashtagPrefix ? this.state.hashtagPrefix : ''} | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| /> | ||||||
| </div> | ||||||
| <div | ||||||
| className='fifty' | ||||||
| style={{padding: '5px'}} | ||||||
| > | ||||||
| <label className='control-label'>{'Date Format'}</label> | ||||||
| <br/> | ||||||
|
There was a problem hiding this comment. Are we able to accomplish this with css instead of using There was a problem hiding this comment. @mickmister removed |
||||||
| <select | ||||||
| name='format' | ||||||
| value={this.state.dateFormat} | ||||||
| onChange={this.handleDateFormat} | ||||||
| style={{height: '35px', border: '1px solid #ced4da'}} | ||||||
|
There was a problem hiding this comment. Can this be done using an There was a problem hiding this comment. Sure, will move all css to scss. |
||||||
| className='form-select' | ||||||
| > | ||||||
| <option value='Jan 2'>{'month_day'}</option> | ||||||
| <option value='2 Jan'>{'day_month'}</option> | ||||||
| <option value='1 2'>{'month_day'}</option> | ||||||
| <option value='2 1'>{'day_month'}</option> | ||||||
| <option value='2006 1 2'>{'year_month_day'}</option> | ||||||
|
There was a problem hiding this comment. Why do the values have spaces instead of underscores? There was a problem hiding this comment. because underscore has special significance for go |
||||||
|
|
||||||
|
There was a problem hiding this comment. This empty line can be removed |
||||||
| </select> | ||||||
| </div> | ||||||
| </div> | ||||||
|
|
||||||
| <p className='text-muted pt-1'> | ||||||
| <div | ||||||
| className='alert alert-warning' | ||||||
| role='alert' | ||||||
| style={{marginBottom: '3px'}} | ||||||
| > | ||||||
| {'Prefixes may use underscore'}<code>{'_'}</code>{'.'} {'Other special characters including'} <code>{'-'}</code> {'are not allowed.'} | ||||||
|
There was a problem hiding this comment. Why is this a requirement? There was a problem hiding this comment. |
||||||
| </div> | ||||||
| {'Date would be appended to Hashtag Prefix, according to the chosen format.'} | ||||||
| </p> | ||||||
| </div> | ||||||
| </Modal.Body> | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did the default change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hashtags with
-were returning no results for me, after some digging found out:ref
2/5, our plugin should be database agnostic but not sure if I should include these changes in this PR.
@mickmister
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to reproduce this issue using postgress using
#foo-02. Is there anything more specific case that causes issues?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case was same, but issue appeared randomly.